Tutorials: OAuth2#3020
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates OAuth2 token issuance and validation to support resource, scope, and issuer claims, adds matching tutorial and integration test coverage, updates JWT scope parsing and JWKS loading, and changes test JVM XML entity limits. ChangesOAuth2/JWT behavior
Tutorials, documentation, and integration tests
Build, example, and misc updates
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
distribution/tutorials/security/50-OAuth2-Client-Credentials.yaml (1)
34-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the end-user credential store from this client-credentials example.
The tutorial text says this flow is machine-to-machine with no username/password, but Lines 34-37 still provision a user store. Even if the test never uses it, that makes the example teach the wrong contract for
client_credentials.Suggested cleanup
- oauth2authserver: issuer: http://localhost:7007 - staticUserDataProvider: - users: - - username: john - password: password staticClientList: clients: - clientId: abc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@distribution/tutorials/security/50-OAuth2-Client-Credentials.yaml` around lines 34 - 37, The OAuth2 client-credentials tutorial still provisions an end-user login store via staticUserDataProvider, which conflicts with the machine-to-machine contract. Remove the users/password block from the example YAML so the 50-OAuth2-Client-Credentials tutorial only demonstrates client_credentials setup and no username/password authentication.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@distribution/tutorials/security/55-OAuth2-Client-Token-Renewal.yaml`:
- Around line 22-50: The OAuth2 client renewal tutorial currently never
exercises token expiry, so it only validates initial acquisition. Update the
OAuth2 auth-server setup in the tutorial config so the access token lifetime is
intentionally short, and adjust OAuth2ClientTokenRenewalTutorialTest to send a
second request after the first token has expired. Use the existing
oauth2authserver and oauth2Client flow blocks as the places to change the
renewal scenario, and keep the test asserting that the client re-fetches a new
token on the second request.
---
Nitpick comments:
In `@distribution/tutorials/security/50-OAuth2-Client-Credentials.yaml`:
- Around line 34-37: The OAuth2 client-credentials tutorial still provisions an
end-user login store via staticUserDataProvider, which conflicts with the
machine-to-machine contract. Remove the users/password block from the example
YAML so the 50-OAuth2-Client-Credentials tutorial only demonstrates
client_credentials setup and no username/password authentication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 36651a06-e4c2-43b6-ad6e-3489dd6aa35b
📒 Files selected for processing (6)
distribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2ClientCredentialsTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2ClientTokenRenewalTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2PasswordFlowTutorialTest.javadistribution/tutorials/security/40-OAuth2-Password-Flow.yamldistribution/tutorials/security/50-OAuth2-Client-Credentials.yamldistribution/tutorials/security/55-OAuth2-Client-Token-Renewal.yaml
|
This pull request needs "/ok-to-test" from an authorized committer. |
Member
|
/ok-to-test |
Member
|
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@distribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2ClientTokenRenewalTutorialTest.java`:
- Around line 87-108: The OAuth2 token renewal test is flaky because
`OAuth2ClientTokenRenewalTutorialTest` relies on fixed `Thread.sleep` timing
around the cache expiry boundary. Replace the sleep-based assertions in
`gatewayRenewsTokenAfterExpiry`/`reusesCachedTokenThenRenewsAfterExpiry` with a
polling wait that checks `countTokenFetches()` and `lastForwardedSuffix()` until
the expected state is reached, or otherwise widen the timing margins
significantly. Keep the checks tied to the existing helper methods so the test
validates cached reuse first and token renewal only after expiry, without
depending on exact scheduler timing.
In `@distribution/tutorials/security/50-OAuth2-Token-Validation.yaml`:
- Around line 61-64: The tutorial comment in the OAuth2 token validation YAML
points to the wrong line for the tokenValidator endpoint. Update the explanatory
text near the token validation step to reference the actual `endpoint:` setting
in the `tokenValidator` block, using the correct location relative to the
`target:` and `endpoint:` keys so readers are directed to the right field.
In `@distribution/tutorials/security/README.md`:
- Around line 12-24: The tutorial index in the security README is missing the
new JWKS flow, so the added 42a/42b tutorials are not discoverable. Update the
list under the tutorial overview and the “Next Steps” section to include the new
JWKS issuer and validation tutorials alongside 40 and 41. Use the existing
tutorial entry style in the README and reference the new filenames
42a-JWKS-Issuer.yaml and 42b-JWKS-Validation.yaml when adding the step 3
guidance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a555bcb1-485a-42eb-acef-bcb1c6b7d5b1
📒 Files selected for processing (17)
distribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2ClientCredentialsTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2ClientTokenRenewalTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2PasswordFlowTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2TokenValidationTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/jwt/IssuingAndValidatingJwtsTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/jwt/JwksIssuerAndValidationTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/jwt/RequestingTokenTutorialTest.javadistribution/tutorials/security/40-JWT-Requesting-Token.mddistribution/tutorials/security/41-JWT-Issuing-and-Validating.yamldistribution/tutorials/security/42a-JWKS-Issuer.yamldistribution/tutorials/security/42b-JWKS-Validation.yamldistribution/tutorials/security/50-OAuth2-Token-Validation.yamldistribution/tutorials/security/51-OAuth2-Password-Flow.yamldistribution/tutorials/security/52-OAuth2-Client-Credentials.yamldistribution/tutorials/security/53-OAuth2-Client-Token-Renewal.yamldistribution/tutorials/security/README.mddocs/ROADMAP.md
✅ Files skipped from review due to trivial changes (2)
- distribution/tutorials/security/42a-JWKS-Issuer.yaml
- distribution/tutorials/security/42b-JWKS-Validation.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- distribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2ClientCredentialsTutorialTest.java
- distribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2PasswordFlowTutorialTest.java
Member
|
test failure |
Client credentials grant: - New <client> attribute "resources" (allowlist, RFC 8707): the "resource" request parameter is validated against it (error "invalid_target"), granted resources become the "aud" claim. Without the parameter, the whole allowlist is granted. - New <client> attribute "scopes" (allowlist): the "scope" request parameter is validated against it (error "invalid_scope"), granted scopes become the "scope" claim (RFC 9068) and the response scope field. Without the attribute, scope is passed through unchecked. - Client lookup and grant type check now run before the token is created; previously a token was created and registered even for invalid grant types. Password grant: - The "scopes" user attribute is passed into the token as "scopes" claim, analogous to the existing "aud" user attribute. Both survive a refresh via i-* claims in the refresh token. Requires a JWT token generator (bearerJwtToken); opaque tokens have no claims. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
RFC 9068 requires access tokens to carry an "iss" claim; so far the configured issuer only appeared in the discovery document and in ID tokens. The authorization server now passes its issuer to the access and refresh token generators, and bearerJwtToken writes it into every token. Opaque bearerToken ignores it (no claims to carry). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A pure client_credentials setup has no users, yet the config forced an empty <staticUserDataProvider/> placeholder. The attribute is no longer @required; without it, startup logs a prominent warning (same pattern as missing location/consentFile), the login view is disabled, and user-based flows answer with "access_denied". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rejections that were previously silent now log at info level: invalid or session-less tokens at the userinfo endpoint, unresolvable refresh tokens, tokenValidator failures, missing JWT headers, and JWTs signed by unknown keys. jwtAuth error responses now include the concrete jose4j validation failure in the detail field; test assertions check the detailed messages. Also fixes a regression introduced with the userinfo log split: the session lookup called getToken() before isValid(), so a malformed Authorization header caused a 500 instead of 401. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
If set, the token's "iss" claim must be present and match, mirroring the existing expectedAud/expectedTid checks. Complements the iss claim now issued by the oauth2authserver's bearerJwtToken. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The security scheme only looked at the "scp" claim (Microsoft Entra ID style), so scopes() and scope checks stayed empty for tokens issued by Membrane's own authorization server, which uses the standard "scope" claim. "scope" is now the fallback when "scp" is absent; "scp" still wins when both are present. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jwtAuth no longer fails hard when the JWKS URIs cannot be resolved at init time (e.g. the issuer starts after the validator). The gateway starts, logs the failure, and fetches the keys on the first request that needs them. Until then, requests validated against the key set are rejected. Periodic refresh keeps the previous key set when a refresh returns no keys. Tutorial 42b no longer claims the issuer must run first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… updated flows and tests Consolidated and renamed tutorials for clarity, aligning with current OAuth2 standards and best practices. Replaced redundant files with streamlined examples, updated JWT claim handling, and adjusted tests for new configurations.
…sweep Added a new test to confirm that refresh tokens remain valid after a cleanup sweep in session management. Updated OAuth2 tutorials to better illustrate refresh token usage and cleanup behavior. Adjusted session initialization to prevent premature token invalidation.
Member
Author
|
@coderabbitai resume reviews |
Contributor
|
✅ Action performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/UserinfoRequest.java (1)
51-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReasonable validation split with clear logging.
Checking
isValid()beforegetToken()correctly avoids the exception path on malformed headers, and the two distinct log messages aid debuggability without leaking token contents.One minor nit:
authHeader.getToken()is called three times (lines 56, 60, 62) after validity is confirmed. Caching it in a local variable once would be slightly cleaner, though not a correctness concern.♻️ Optional cleanup
- if(!authServer.getSessionFinder().hasSessionForToken(authHeader.getToken())) { + String token = authHeader.getToken(); + if(!authServer.getSessionFinder().hasSessionForToken(token)) { log.info("Access token at userinfo endpoint not accepted: token is valid but has no associated session."); return buildWwwAuthenticateErrorResponse( Response.unauthorized(), "invalid_token"); } - sessionProperties = new HashMap<>(authServer.getSessionFinder().getSessionForToken(authHeader.getToken()).getUserAttributes()); - - String token = authHeader.getToken(); + sessionProperties = new HashMap<>(authServer.getSessionFinder().getSessionForToken(token).getUserAttributes());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/UserinfoRequest.java` around lines 51 - 64, Cache the result of authHeader.getToken() in UserinfoRequest after the isValid() check instead of calling it multiple times. Use the local token variable for the hasSessionForToken/getSessionForToken lookup and for getUsername, keeping the existing validation order and log messages intact.distribution/tutorials/security/54b-OAuth2-Distributed-Validation.yaml (1)
33-35: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider demonstrating
expectedIssgiven this PR's new issuer-validation feature.This same PR adds
expectedIsssupport tojwtAuth(seeJwtAuthInterceptor.createValidator), and 54a explicitly sets anissuerand emits anissclaim, yet this validator doesn't configureexpectedIssto check it. For a tutorial specifically about cross-instance trust between issuer and validator, showingexpectedIss: http://localhost:7007would better demonstrate/exercise the just-added feature and improve the security posture of the example.Suggested addition
- jwtAuth: jwks: jwksUris: http://localhost:7007/oauth2/certs + expectedIss: http://localhost:7007🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@distribution/tutorials/security/54b-OAuth2-Distributed-Validation.yaml` around lines 33 - 35, The `jwtAuth` example in the distributed validation tutorial is missing the new issuer check, so update the tutorial YAML to demonstrate `expectedIss` alongside `jwksUris`. Use the `jwtAuth` configuration in this tutorial to add `expectedIss: http://localhost:7007`, matching the issuer shown in the paired 54a example and exercising `JwtAuthInterceptor.createValidator`’s new issuer-validation support.distribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2DistributedValidationTutorialTest.java (1)
58-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTest only covers issuer-first startup; consider adding the reversed-order case.
The class doc explains that The JWTS URIs may be unreachable during init() (e.g. Membrane starts before the issuer); init() only logs that failure, so the load is retried here on first use. The tutorial's own comments (54b, lines 31-32) explicitly advertise that the validator "still starts and fetches the keys on the first request" if the issuer isn't up yet — but no test exercises that retry path (starting the validator before the issuer). Adding a second test case would give coverage for the documented resiliency behavior this tutorial highlights.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@distribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2DistributedValidationTutorialTest.java` around lines 58 - 91, Add coverage for the retry path where the JWT validator starts before the issuer, since the current OAuth2DistributedValidationTutorialTest only verifies issuer-first startup. Introduce a second test in this class that brings up the validator/protected API first, confirms the initial key fetch can fail during init(), then starts the issuer and asserts the first authenticated request still succeeds by fetching JWKS on demand. Use the existing issuesJwtAndValidatesViaJwks flow and the tutorial’s documented startup order behavior as the reference point.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthServerWithoutUserDataProviderTest.java (1)
89-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared test scaffolding across new OAuth2 tests.
The
tokenRequest()helper,BearerJwtTokenGenerator/RefreshTokenConfigwiring, and license header are duplicated near-verbatim inCredentialsFlowScopeTest,PasswordFlowClaimsTest, and (per graph context)CredentialsFlowResourceTest. Consider extracting a shared abstract base test class (e.g.OAuth2TokenRequestTestBase) withtokenRequest()/accessTokenClaims()and commonoasisetup, to avoid drift as more OAuth2 tutorial tests are added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthServerWithoutUserDataProviderTest.java` around lines 89 - 100, The OAuth2 test setup is duplicated across the new flow tests, so extract the shared scaffolding into a common abstract base such as OAuth2TokenRequestTestBase. Move the repeated tokenRequest() helper, the common oasi initialization, and the BearerJwtTokenGenerator/RefreshTokenConfig wiring into that base, then have CredentialsFlowScopeTest, PasswordFlowClaimsTest, and CredentialsFlowResourceTest inherit from it and keep only test-specific assertions like accessTokenClaims(). Also centralize the shared license header there so the tests stay consistent and easier to maintain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 111-125: The on-demand JWKS refresh in Jwks.reloadJwksIfNeeded()
is unsynchronized, so concurrent requests can all enter loadJwks(false) when
jwks is empty and hammer the JWKS endpoint. Add coordination in
reloadJwksIfNeeded()/getJwks() (for example a lock plus a last-attempt
timestamp/backoff) so only one request thread performs the blocking reload and
rapid failures do not trigger repeated network fetches on every call.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.java`:
- Around line 131-140: The catch-all in JwtAuthInterceptor should not expose raw
exception messages to clients; it currently passes e.getMessage() into the
response detail, which can leak internal JWKS/config information. Update this
exception path to use a fixed generic detail string like the other
JwtAuthInterceptor handlers, keep the specific e.getMessage() only in the server
log, and leave the response construction in
security(...).detail(...).addSubSee(...) consistent with the existing JWT error
handling.
In `@distribution/tutorials/security/54a-OAuth2-Distributed-Issuer.yaml`:
- Around line 20-26: The tutorial text and the JWT claims configuration are
inconsistent: the walkthrough mentions noting `clientId`, but `claims.value` in
the distributed issuer setup only emits `sub` and `iss`. Update either the
comment in the tutorial section or the claims definition so they match, and use
the existing `claims.value`/JWT walkthrough wording to keep the step-by-step
instructions aligned with the actual token contents.
In `@docs/ROADMAP.md`:
- Line 123: The sample command in the roadmap still references the old tutorial
filename, so update the example in the roadmap content to use the new
`54b-OAuth2-Distributed-Validation.yaml` name instead of
`54b-OpenID-Connect-Login.yaml`. Make the change in the existing command snippet
so readers copying it will run the correct tutorial file.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/UserinfoRequest.java`:
- Around line 51-64: Cache the result of authHeader.getToken() in
UserinfoRequest after the isValid() check instead of calling it multiple times.
Use the local token variable for the hasSessionForToken/getSessionForToken
lookup and for getUsername, keeping the existing validation order and log
messages intact.
In
`@core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthServerWithoutUserDataProviderTest.java`:
- Around line 89-100: The OAuth2 test setup is duplicated across the new flow
tests, so extract the shared scaffolding into a common abstract base such as
OAuth2TokenRequestTestBase. Move the repeated tokenRequest() helper, the common
oasi initialization, and the BearerJwtTokenGenerator/RefreshTokenConfig wiring
into that base, then have CredentialsFlowScopeTest, PasswordFlowClaimsTest, and
CredentialsFlowResourceTest inherit from it and keep only test-specific
assertions like accessTokenClaims(). Also centralize the shared license header
there so the tests stay consistent and easier to maintain.
In
`@distribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2DistributedValidationTutorialTest.java`:
- Around line 58-91: Add coverage for the retry path where the JWT validator
starts before the issuer, since the current
OAuth2DistributedValidationTutorialTest only verifies issuer-first startup.
Introduce a second test in this class that brings up the validator/protected API
first, confirms the initial key fetch can fail during init(), then starts the
issuer and asserts the first authenticated request still succeeds by fetching
JWKS on demand. Use the existing issuesJwtAndValidatesViaJwks flow and the
tutorial’s documented startup order behavior as the reference point.
In `@distribution/tutorials/security/54b-OAuth2-Distributed-Validation.yaml`:
- Around line 33-35: The `jwtAuth` example in the distributed validation
tutorial is missing the new issuer check, so update the tutorial YAML to
demonstrate `expectedIss` alongside `jwksUris`. Use the `jwtAuth` configuration
in this tutorial to add `expectedIss: http://localhost:7007`, matching the
issuer shown in the paired 54a example and exercising
`JwtAuthInterceptor.createValidator`’s new issuer-validation support.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 628073c5-8cf6-4eb9-ad38-581849238ecd
📒 Files selected for processing (38)
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/SessionManager.javacore/src/main/java/com/predic8/membrane/core/interceptor/jwt/HeaderJwtRetriever.javacore/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.javacore/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/Client.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2AuthorizationServerInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/ParamNames.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/ParameterizedRequest.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/UserinfoRequest.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/CredentialsFlow.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/RefreshTokenFlow.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/BearerJwtTokenGenerator.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/TokenGenerator.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokenvalidation/OAuth2TokenValidatorInterceptor.javacore/src/main/java/com/predic8/membrane/core/security/JWTSecurityScheme.javacore/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/security/JWTSecuritySchemeTest.javacore/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthServerWithoutUserDataProviderTest.javacore/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/CredentialsFlowResourceTest.javacore/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/CredentialsFlowScopeTest.javacore/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/PasswordFlowClaimsTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2BasicsTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2ClientCredentialsTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2ClientTokenRenewalTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2DistributedValidationTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2PasswordFlowTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/jwt/JwtSigningTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/jwt/RequestingTokenTutorialTest.javadistribution/tutorials/security/40-JWT-Requesting-Token.mddistribution/tutorials/security/41-JWT-Signing.yamldistribution/tutorials/security/50-OAuth2-Basics.yamldistribution/tutorials/security/51-OAuth2-Client-Credentials.yamldistribution/tutorials/security/52-OAuth2-Password-Flow.yamldistribution/tutorials/security/53-OAuth2-Client-Token-Renewal.yamldistribution/tutorials/security/54a-OAuth2-Distributed-Issuer.yamldistribution/tutorials/security/54b-OAuth2-Distributed-Validation.yamldistribution/tutorials/security/README.mddocs/ROADMAP.md
✅ Files skipped from review due to trivial changes (4)
- distribution/tutorials/security/51-OAuth2-Client-Credentials.yaml
- core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokenvalidation/OAuth2TokenValidatorInterceptor.java
- core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/RefreshTokenFlow.java
- distribution/tutorials/security/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- distribution/src/test/java/com/predic8/membrane/tutorials/security/jwt/RequestingTokenTutorialTest.java
- distribution/tutorials/security/53-OAuth2-Client-Token-Renewal.yaml
- distribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2ClientTokenRenewalTutorialTest.java
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🧹 Nitpick comments (4)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/UserinfoRequest.java (1)
51-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReasonable validation split with clear logging.
Checking
isValid()beforegetToken()correctly avoids the exception path on malformed headers, and the two distinct log messages aid debuggability without leaking token contents.One minor nit:
authHeader.getToken()is called three times (lines 56, 60, 62) after validity is confirmed. Caching it in a local variable once would be slightly cleaner, though not a correctness concern.♻️ Optional cleanup
- if(!authServer.getSessionFinder().hasSessionForToken(authHeader.getToken())) { + String token = authHeader.getToken(); + if(!authServer.getSessionFinder().hasSessionForToken(token)) { log.info("Access token at userinfo endpoint not accepted: token is valid but has no associated session."); return buildWwwAuthenticateErrorResponse( Response.unauthorized(), "invalid_token"); } - sessionProperties = new HashMap<>(authServer.getSessionFinder().getSessionForToken(authHeader.getToken()).getUserAttributes()); - - String token = authHeader.getToken(); + sessionProperties = new HashMap<>(authServer.getSessionFinder().getSessionForToken(token).getUserAttributes());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/UserinfoRequest.java` around lines 51 - 64, Cache the result of authHeader.getToken() in UserinfoRequest after the isValid() check instead of calling it multiple times. Use the local token variable for the hasSessionForToken/getSessionForToken lookup and for getUsername, keeping the existing validation order and log messages intact.distribution/tutorials/security/54b-OAuth2-Distributed-Validation.yaml (1)
33-35: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider demonstrating
expectedIssgiven this PR's new issuer-validation feature.This same PR adds
expectedIsssupport tojwtAuth(seeJwtAuthInterceptor.createValidator), and 54a explicitly sets anissuerand emits anissclaim, yet this validator doesn't configureexpectedIssto check it. For a tutorial specifically about cross-instance trust between issuer and validator, showingexpectedIss: http://localhost:7007would better demonstrate/exercise the just-added feature and improve the security posture of the example.Suggested addition
- jwtAuth: jwks: jwksUris: http://localhost:7007/oauth2/certs + expectedIss: http://localhost:7007🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@distribution/tutorials/security/54b-OAuth2-Distributed-Validation.yaml` around lines 33 - 35, The `jwtAuth` example in the distributed validation tutorial is missing the new issuer check, so update the tutorial YAML to demonstrate `expectedIss` alongside `jwksUris`. Use the `jwtAuth` configuration in this tutorial to add `expectedIss: http://localhost:7007`, matching the issuer shown in the paired 54a example and exercising `JwtAuthInterceptor.createValidator`’s new issuer-validation support.distribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2DistributedValidationTutorialTest.java (1)
58-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTest only covers issuer-first startup; consider adding the reversed-order case.
The class doc explains that The JWTS URIs may be unreachable during init() (e.g. Membrane starts before the issuer); init() only logs that failure, so the load is retried here on first use. The tutorial's own comments (54b, lines 31-32) explicitly advertise that the validator "still starts and fetches the keys on the first request" if the issuer isn't up yet — but no test exercises that retry path (starting the validator before the issuer). Adding a second test case would give coverage for the documented resiliency behavior this tutorial highlights.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@distribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2DistributedValidationTutorialTest.java` around lines 58 - 91, Add coverage for the retry path where the JWT validator starts before the issuer, since the current OAuth2DistributedValidationTutorialTest only verifies issuer-first startup. Introduce a second test in this class that brings up the validator/protected API first, confirms the initial key fetch can fail during init(), then starts the issuer and asserts the first authenticated request still succeeds by fetching JWKS on demand. Use the existing issuesJwtAndValidatesViaJwks flow and the tutorial’s documented startup order behavior as the reference point.core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthServerWithoutUserDataProviderTest.java (1)
89-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared test scaffolding across new OAuth2 tests.
The
tokenRequest()helper,BearerJwtTokenGenerator/RefreshTokenConfigwiring, and license header are duplicated near-verbatim inCredentialsFlowScopeTest,PasswordFlowClaimsTest, and (per graph context)CredentialsFlowResourceTest. Consider extracting a shared abstract base test class (e.g.OAuth2TokenRequestTestBase) withtokenRequest()/accessTokenClaims()and commonoasisetup, to avoid drift as more OAuth2 tutorial tests are added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthServerWithoutUserDataProviderTest.java` around lines 89 - 100, The OAuth2 test setup is duplicated across the new flow tests, so extract the shared scaffolding into a common abstract base such as OAuth2TokenRequestTestBase. Move the repeated tokenRequest() helper, the common oasi initialization, and the BearerJwtTokenGenerator/RefreshTokenConfig wiring into that base, then have CredentialsFlowScopeTest, PasswordFlowClaimsTest, and CredentialsFlowResourceTest inherit from it and keep only test-specific assertions like accessTokenClaims(). Also centralize the shared license header there so the tests stay consistent and easier to maintain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java`:
- Around line 111-125: The on-demand JWKS refresh in Jwks.reloadJwksIfNeeded()
is unsynchronized, so concurrent requests can all enter loadJwks(false) when
jwks is empty and hammer the JWKS endpoint. Add coordination in
reloadJwksIfNeeded()/getJwks() (for example a lock plus a last-attempt
timestamp/backoff) so only one request thread performs the blocking reload and
rapid failures do not trigger repeated network fetches on every call.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.java`:
- Around line 131-140: The catch-all in JwtAuthInterceptor should not expose raw
exception messages to clients; it currently passes e.getMessage() into the
response detail, which can leak internal JWKS/config information. Update this
exception path to use a fixed generic detail string like the other
JwtAuthInterceptor handlers, keep the specific e.getMessage() only in the server
log, and leave the response construction in
security(...).detail(...).addSubSee(...) consistent with the existing JWT error
handling.
In `@distribution/tutorials/security/54a-OAuth2-Distributed-Issuer.yaml`:
- Around line 20-26: The tutorial text and the JWT claims configuration are
inconsistent: the walkthrough mentions noting `clientId`, but `claims.value` in
the distributed issuer setup only emits `sub` and `iss`. Update either the
comment in the tutorial section or the claims definition so they match, and use
the existing `claims.value`/JWT walkthrough wording to keep the step-by-step
instructions aligned with the actual token contents.
In `@docs/ROADMAP.md`:
- Line 123: The sample command in the roadmap still references the old tutorial
filename, so update the example in the roadmap content to use the new
`54b-OAuth2-Distributed-Validation.yaml` name instead of
`54b-OpenID-Connect-Login.yaml`. Make the change in the existing command snippet
so readers copying it will run the correct tutorial file.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/UserinfoRequest.java`:
- Around line 51-64: Cache the result of authHeader.getToken() in
UserinfoRequest after the isValid() check instead of calling it multiple times.
Use the local token variable for the hasSessionForToken/getSessionForToken
lookup and for getUsername, keeping the existing validation order and log
messages intact.
In
`@core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthServerWithoutUserDataProviderTest.java`:
- Around line 89-100: The OAuth2 test setup is duplicated across the new flow
tests, so extract the shared scaffolding into a common abstract base such as
OAuth2TokenRequestTestBase. Move the repeated tokenRequest() helper, the common
oasi initialization, and the BearerJwtTokenGenerator/RefreshTokenConfig wiring
into that base, then have CredentialsFlowScopeTest, PasswordFlowClaimsTest, and
CredentialsFlowResourceTest inherit from it and keep only test-specific
assertions like accessTokenClaims(). Also centralize the shared license header
there so the tests stay consistent and easier to maintain.
In
`@distribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2DistributedValidationTutorialTest.java`:
- Around line 58-91: Add coverage for the retry path where the JWT validator
starts before the issuer, since the current
OAuth2DistributedValidationTutorialTest only verifies issuer-first startup.
Introduce a second test in this class that brings up the validator/protected API
first, confirms the initial key fetch can fail during init(), then starts the
issuer and asserts the first authenticated request still succeeds by fetching
JWKS on demand. Use the existing issuesJwtAndValidatesViaJwks flow and the
tutorial’s documented startup order behavior as the reference point.
In `@distribution/tutorials/security/54b-OAuth2-Distributed-Validation.yaml`:
- Around line 33-35: The `jwtAuth` example in the distributed validation
tutorial is missing the new issuer check, so update the tutorial YAML to
demonstrate `expectedIss` alongside `jwksUris`. Use the `jwtAuth` configuration
in this tutorial to add `expectedIss: http://localhost:7007`, matching the
issuer shown in the paired 54a example and exercising
`JwtAuthInterceptor.createValidator`’s new issuer-validation support.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 628073c5-8cf6-4eb9-ad38-581849238ecd
📒 Files selected for processing (38)
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/SessionManager.javacore/src/main/java/com/predic8/membrane/core/interceptor/jwt/HeaderJwtRetriever.javacore/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.javacore/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/Client.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2AuthorizationServerInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/ParamNames.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/ParameterizedRequest.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/UserinfoRequest.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/CredentialsFlow.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/RefreshTokenFlow.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/BearerJwtTokenGenerator.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/TokenGenerator.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokenvalidation/OAuth2TokenValidatorInterceptor.javacore/src/main/java/com/predic8/membrane/core/security/JWTSecurityScheme.javacore/src/test/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/security/JWTSecuritySchemeTest.javacore/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/AuthServerWithoutUserDataProviderTest.javacore/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/CredentialsFlowResourceTest.javacore/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/CredentialsFlowScopeTest.javacore/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/PasswordFlowClaimsTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2BasicsTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2ClientCredentialsTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2ClientTokenRenewalTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2DistributedValidationTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2PasswordFlowTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/jwt/JwtSigningTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/jwt/RequestingTokenTutorialTest.javadistribution/tutorials/security/40-JWT-Requesting-Token.mddistribution/tutorials/security/41-JWT-Signing.yamldistribution/tutorials/security/50-OAuth2-Basics.yamldistribution/tutorials/security/51-OAuth2-Client-Credentials.yamldistribution/tutorials/security/52-OAuth2-Password-Flow.yamldistribution/tutorials/security/53-OAuth2-Client-Token-Renewal.yamldistribution/tutorials/security/54a-OAuth2-Distributed-Issuer.yamldistribution/tutorials/security/54b-OAuth2-Distributed-Validation.yamldistribution/tutorials/security/README.mddocs/ROADMAP.md
✅ Files skipped from review due to trivial changes (4)
- distribution/tutorials/security/51-OAuth2-Client-Credentials.yaml
- core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokenvalidation/OAuth2TokenValidatorInterceptor.java
- core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/RefreshTokenFlow.java
- distribution/tutorials/security/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- distribution/src/test/java/com/predic8/membrane/tutorials/security/jwt/RequestingTokenTutorialTest.java
- distribution/tutorials/security/53-OAuth2-Client-Token-Renewal.yaml
- distribution/src/test/java/com/predic8/membrane/tutorials/security/OAuth2ClientTokenRenewalTutorialTest.java
🛑 Comments failed to post (4)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java (1)
111-125: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unsynchronized on-demand JWKS reload can cause a request-thread thundering herd.
reloadJwksIfNeeded()is invoked on everygetKeyByKid()/getJwks()call whilejwksis empty, and performs a synchronous, blocking HTTP fetch with no cooldown or synchronization. If the JWKS URI stays unreachable, every incoming request needing validation re-attempts the fetch on its own request thread — a thundering herd against a struggling auth server, and added latency/timeout risk per request. The check-then-act on thejwksfield is also not atomic, so concurrent requests can race intoloadJwks()simultaneously.Consider adding a lightweight last-attempt timestamp/backoff (or a lock) so concurrent/rapid retries don't all hit the network simultaneously while the source is down.
Also applies to: 149-149
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java` around lines 111 - 125, The on-demand JWKS refresh in Jwks.reloadJwksIfNeeded() is unsynchronized, so concurrent requests can all enter loadJwks(false) when jwks is empty and hammer the JWKS endpoint. Add coordination in reloadJwksIfNeeded()/getJwks() (for example a lock plus a last-attempt timestamp/backoff) so only one request thread performs the blocking reload and rapid failures do not trigger repeated network fetches on every call.core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.java (1)
131-140: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Generic exception handler now leaks raw internal messages to clients.
The catch-all now returns
e.getMessage()as the responsedetailinstead of a fixed string. This branch catches any unclassified exception — including failures propagated fromJwks.getKeyByKid()/reloadJwksIfNeeded(), whoseRuntimeException/ConfigurationExceptionmessages embed the configuredjwksUris(seeJwks.java'sparseJwksUriIntoList, e.g."Could not retrieve JWK keys from %s."). That means an unauthenticated client hitting this endpoint while the JWKS/auth server is down could receive the internal JWKS URL in the 401 response body. It's also fragile for exceptions with anullmessage (e.g. NPEs), andaddSubSee(ERROR_JWT_NOT_FOUND_ID)stays hard-coded even though the underlying failure may be unrelated to "JWT not found."Consider keeping a generic, fixed detail message for this catch-all (logging
e.getMessage()server-side only), similar to how the other catch blocks use fixed constants.🔒 Suggested fix
} catch (Exception e) { log.info("Could not retrieve JWT: {}", e.getMessage()); security(router.getConfiguration().isProduction(), "jwt-auth") - .detail(e.getMessage()) + .detail(ERROR_JWT_NOT_FOUND) .addSubSee(ERROR_JWT_NOT_FOUND_ID) .stacktrace(true) .status(401) .buildAndSetResponse(exc); return RETURN; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.} catch (Exception e) { log.info("Could not retrieve JWT: {}", e.getMessage()); security(router.getConfiguration().isProduction(), "jwt-auth") .detail(ERROR_JWT_NOT_FOUND) .addSubSee(ERROR_JWT_NOT_FOUND_ID) .stacktrace(true) .status(401) .buildAndSetResponse(exc); return RETURN; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.java` around lines 131 - 140, The catch-all in JwtAuthInterceptor should not expose raw exception messages to clients; it currently passes e.getMessage() into the response detail, which can leak internal JWKS/config information. Update this exception path to use a fixed generic detail string like the other JwtAuthInterceptor handlers, keep the specific e.getMessage() only in the server log, and leave the response construction in security(...).detail(...).addSubSee(...) consistent with the existing JWT error handling.distribution/tutorials/security/54a-OAuth2-Distributed-Issuer.yaml (1)
20-26: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Doc/config mismatch:
clientIdmentioned but not inclaims.value.The walkthrough comment says to "Note sub, clientId, iss, exp and the kid "membrane" in the header." but
claims.value(Line 56) only listssub iss. Following the tutorial's own instructions, a reader inspecting the decoded token at jwt.io won't find aclientIdclaim, which is confusing for a tutorial meant to be followed step-by-step.Suggested fix (align comment or config)
- claims: - value: sub iss + claims: + value: sub iss clientIdor update the comment to drop the
clientIdreference if it isn't meant to be emitted.Also applies to: 55-56
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@distribution/tutorials/security/54a-OAuth2-Distributed-Issuer.yaml` around lines 20 - 26, The tutorial text and the JWT claims configuration are inconsistent: the walkthrough mentions noting `clientId`, but `claims.value` in the distributed issuer setup only emits `sub` and `iss`. Update either the comment in the tutorial section or the claims definition so they match, and use the existing `claims.value`/JWT walkthrough wording to keep the step-by-step instructions aligned with the actual token contents.docs/ROADMAP.md (1)
123-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the sample command to the new tutorial filename.
The example still points at
54b-OpenID-Connect-Login.yaml, but this PR introduces54b-OAuth2-Distributed-Validation.yamlinstead. Readers copying the command will hit a missing file unless this is intentionally referencing an older tutorial.Fix
-❯ ./membrane.sh -c 54b-OpenID-Connect-Login.yaml +❯ ./membrane.sh -c 54b-OAuth2-Distributed-Validation.yaml📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.❯ ./membrane.sh -c 54b-OAuth2-Distributed-Validation.yaml🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ROADMAP.md` at line 123, The sample command in the roadmap still references the old tutorial filename, so update the example in the roadmap content to use the new `54b-OAuth2-Distributed-Validation.yaml` name instead of `54b-OpenID-Connect-Login.yaml`. Make the change in the existing command snippet so readers copying it will run the correct tutorial file.
Introduced refresh token rotation per OAuth2 Security BCP, ensuring consumed tokens are invalidated, and only newly issued tokens remain valid. Added tests to enforce single-use refresh token behavior. Updated tutorials and documentation for TLS best practices.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/PasswordFlow.java (1)
54-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBroad
catch (Exception e)swallows unrelated failures asinvalid_client.Catching
Exceptionhere will also swallow unexpected runtime errors (e.g. NPE, config issues) fromgetClientList(), mapping them to a misleadinginvalid_clientresponse instead of surfacing the real cause.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/PasswordFlow.java` around lines 54 - 56, The broad catch in PasswordFlow’s token handling is masking unrelated failures as invalid_client. Narrow the exception handling around getClientList() and related client validation so only expected authentication/client lookup failures are converted to that OAuth2 error, and let unexpected runtime issues (for example null/config problems) propagate or be handled separately. Use the PasswordFlow method and the getClientList() call site to locate the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/PasswordFlow.java`:
- Around line 54-56: The broad catch in PasswordFlow’s token handling is masking
unrelated failures as invalid_client. Narrow the exception handling around
getClientList() and related client validation so only expected
authentication/client lookup failures are converted to that OAuth2 error, and
let unexpected runtime issues (for example null/config problems) propagate or be
handled separately. Use the PasswordFlow method and the getClientList() call
site to locate the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3025b068-23a6-4c76-af79-4abf5c5d6336
📒 Files selected for processing (22)
core/pom.xmlcore/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/SessionFinder.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/CredentialsFlow.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/PasswordFlow.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/RefreshTokenFlow.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/BearerJwtTokenGenerator.javacore/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/PasswordFlowClaimsTest.javadistribution/conf/log4j2.xmldistribution/pom.xmldistribution/src/test/java/com/predic8/membrane/tutorials/security/jwt/RequestingTokenTutorialTest.javadistribution/tutorials/security/41-JWT-Signing.yamldistribution/tutorials/security/50-OAuth2-Basics.yamldistribution/tutorials/security/51-OAuth2-Client-Credentials.yamldistribution/tutorials/security/52-OAuth2-Password-Flow.yamldistribution/tutorials/security/53-OAuth2-Client-Token-Renewal.yamldistribution/tutorials/security/54a-OAuth2-Distributed-Issuer.yamldistribution/tutorials/security/54b-OAuth2-Distributed-Validation.yamldocs/RELEASE_NOTES.mddocs/ROADMAP.mdpom.xmlwar/pom.xml
💤 Files with no reviewable changes (1)
- distribution/conf/log4j2.xml
✅ Files skipped from review due to trivial changes (3)
- docs/ROADMAP.md
- docs/RELEASE_NOTES.md
- core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/RefreshTokenFlow.java
🚧 Files skipped from review as they are similar to previous changes (11)
- distribution/tutorials/security/50-OAuth2-Basics.yaml
- distribution/tutorials/security/54a-OAuth2-Distributed-Issuer.yaml
- distribution/tutorials/security/54b-OAuth2-Distributed-Validation.yaml
- distribution/tutorials/security/53-OAuth2-Client-Token-Renewal.yaml
- core/src/test/java/com/predic8/membrane/integration/withoutinternet/interceptor/oauth2/PasswordFlowClaimsTest.java
- distribution/tutorials/security/51-OAuth2-Client-Credentials.yaml
- distribution/tutorials/security/52-OAuth2-Password-Flow.yaml
- distribution/tutorials/security/41-JWT-Signing.yaml
- core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/BearerJwtTokenGenerator.java
- core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java
- core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/CredentialsFlow.java
rrayst
requested changes
Jul 3, 2026
Introduced a `scopesClaim` attribute to customize the claim used for scope validation in JWTSecurityScheme (e.g., "scp" or "scope"). Updated JwtAuthInterceptor to make use of this attribute. Refactored and expanded test cases for improved clarity and coverage, ensuring proper handling of scope claims.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/com/predic8/membrane/core/security/JWTSecurityScheme.java (1)
29-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard scope splitting against leading/blank tokens.
" +"only trims trailing empty tokens, so a leading space (e.g." read write") yields an empty-string scope, and a blank claim value produces{""}. Consider trimming and filtering empties. Also, the secondifis logically anelse ifsince a value can't be bothStringandList.♻️ Proposed fix
- var scopes = jwt.get(scopesClaim); - if (scopes instanceof String scopeString) { - this.scopes = new HashSet<>(Arrays.asList(scopeString.split(" +"))); - } - if (scopes instanceof List<?> scopeList) { - this.scopes = new HashSet<>(scopeList.stream().map(Object::toString).toList()); - } + var scopes = jwt.get(scopesClaim); + if (scopes instanceof String scopeString) { + this.scopes = Arrays.stream(scopeString.trim().split(" +")) + .filter(s -> !s.isEmpty()) + .collect(Collectors.toCollection(HashSet::new)); + } else if (scopes instanceof List<?> scopeList) { + this.scopes = new HashSet<>(scopeList.stream().map(Object::toString).toList()); + }(requires
import java.util.stream.Collectors;)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/predic8/membrane/core/security/JWTSecurityScheme.java` around lines 29 - 37, In JWTSecurityScheme’s constructor, scope parsing can create empty-string scopes for leading or blank values, and the two type checks should not both run for the same claim. Update the String handling to trim the claim and filter out empty tokens before building the HashSet, and change the second scopes instanceof List<?> check to an else if so only one branch applies. Use the existing scopes field and jwt/scopesClaim lookup to keep the fix localized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@core/src/main/java/com/predic8/membrane/core/security/JWTSecurityScheme.java`:
- Around line 29-37: In JWTSecurityScheme’s constructor, scope parsing can
create empty-string scopes for leading or blank values, and the two type checks
should not both run for the same claim. Update the String handling to trim the
claim and filter out empty tokens before building the HashSet, and change the
second scopes instanceof List<?> check to an else if so only one branch applies.
Use the existing scopes field and jwt/scopesClaim lookup to keep the fix
localized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c092261-cba5-4b0b-bb9c-7d685b6d6b0d
📒 Files selected for processing (11)
core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.javacore/src/main/java/com/predic8/membrane/core/interceptor/jwt/JwtAuthInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/BearerJwtTokenGenerator.javacore/src/main/java/com/predic8/membrane/core/security/JWTSecurityScheme.javacore/src/test/java/com/predic8/membrane/core/security/JWTSecuritySchemeTest.javadistribution/release-notes/0.0.0.mddistribution/release-notes/5.1.17.mddistribution/release-notes/5.1.18.mddistribution/release-notes/5.1.19.mddistribution/tutorials/security/40-JWT-Requesting-Token.mddocs/RELEASE_NOTES.md
💤 Files with no reviewable changes (5)
- distribution/release-notes/5.1.19.md
- distribution/release-notes/5.1.17.md
- distribution/release-notes/0.0.0.md
- distribution/release-notes/5.1.18.md
- docs/RELEASE_NOTES.md
✅ Files skipped from review due to trivial changes (1)
- distribution/tutorials/security/40-JWT-Requesting-Token.md
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/tokengenerators/BearerJwtTokenGenerator.java
- core/src/test/java/com/predic8/membrane/core/security/JWTSecuritySchemeTest.java
- core/src/main/java/com/predic8/membrane/core/interceptor/jwt/Jwks.java
# Conflicts: # docs/RELEASE_NOTES.md
rrayst
approved these changes
Jul 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
iss), audience (aud), and scope claim parsing (scp/scope), plusresourceandscopeallowlist validation.