OAuth2 improvements #2091
OAuth2 improvements #2091
Conversation
* assert that OAuth2 error during second flow logs user out * added configuration option to copy client_id parameter to token endpoint's request body * renamed clientIdInBody='true' to clientAuthorization='client_secret_post' to align config with OIDC terminology * refactoring to use ParameterNames.* constants
WalkthroughRefactors OAuth2 token request construction to use a builder (OAuth2TokenBody), introduces Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant AS as AuthorizationService
participant TB as OAuth2TokenBody
participant TE as TokenEndpoint
Client->>AS: request token (flowContext)
AS->>TB: compose grant params (grant_type, code/refresh_token, redirect_uri, scope...)
alt client_assertion provided
AS->>TB: clientAssertion(type, assertion)
end
alt ClientAuthorization = client_secret_basic
AS->>AS: create Authorization: Basic(clientId:clientSecret)
AS->>TE: POST /token with Authorization header and TB.build()
else ClientAuthorization = client_secret_post
AS->>TB: clientId(id).clientSecret(secret)
AS->>TE: POST /token with TB.build() (credentials in body)
else no clientSecret
AS->>TB: clientId(id)
AS->>TE: POST /token with TB.build() (no secret)
end
TE-->>AS: token response / error
AS-->>Client: propagate result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/ClientAuthorization.java (1)
3-11: Enum values intentionally mirror spec tokens; add rationale and plan for future methodsUsing lowercase enum constants matches OIDC/OAuth2 token_endpoint_auth_method values and is convenient when serialized. Add a brief note that these identifiers are intentionally aligned with the spec to justify deviating from Java’s usual UPPER_SNAKE_CASE, and consider outlining whether support for client_secret_jwt/private_key_jwt is planned (the rest of the PR already introduces client assertion fields).
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2TokenBody.java (3)
56-70: Guard against contradictory client auth inputsRight now callers can set both clientSecret and clientAssertion(_Type). That combination is invalid for a single request. Add a simple validation (either here or in build()) to fail fast if both methods are configured.
Apply this diff inside build() (see lines 72-84) to reject invalid combinations:
+ if ((clientAssertionType != null || clientAssertion != null) && clientSecret != null) { + throw new IllegalStateException("Specify either client_secret (client_secret_basic/post) or client_assertion(_type) (private_key_jwt/client_secret_jwt), not both."); + } + if ((clientAssertionType == null) != (clientAssertion == null)) { + throw new IllegalStateException("client_assertion_type and client_assertion must be provided together."); + }
72-84: Normalize form-encoding; prefer URLParamUtil.ParamBuilder over manual concatenationManual concatenation partially encodes only scope, leaving other values (e.g., redirect_uri) unencoded. For correctness and maintainability, build the body with URLParamUtil.ParamBuilder, which percent-encodes consistently. This also lets you drop appendParam helpers.
Apply this diff:
- public String build() { - StringBuilder r = new StringBuilder("grant_type=" + grantType); - appendParam(r, "refresh_token", refreshToken); - appendParam(r, "code", code); - appendParam(r, "redirect_uri", redirectUri); - appendParam(r, "scope", scope, e -> encode(e, UTF_8)); - appendParam(r, "code_verifier", codeVerifier); - appendParam(r, "client_id", clientId); - appendParam(r, "client_secret", clientSecret); - appendParam(r, "client_assertion_type", clientAssertionType); - appendParam(r, "client_assertion", clientAssertion); - return r.toString(); - } + public String build() { + if (grantType == null) + throw new IllegalStateException("grant_type must be set."); + + // disallow contradictory/malformed client auth (see separate comment) + if ((clientAssertionType != null || clientAssertion != null) && clientSecret != null) + throw new IllegalStateException("Specify either client_secret or client_assertion(_type), not both."); + if ((clientAssertionType == null) != (clientAssertion == null)) + throw new IllegalStateException("client_assertion_type and client_assertion must be provided together."); + + URLParamUtil.ParamBuilder pb = new URLParamUtil.ParamBuilder(); + pb.add("grant_type", grantType); + if (refreshToken != null) pb.add("refresh_token", refreshToken); + if (code != null) pb.add("code", code); + if (redirectUri != null) pb.add("redirect_uri", redirectUri); + if (scope != null) pb.add("scope", scope); + if (codeVerifier != null) pb.add("code_verifier", codeVerifier); + if (clientId != null) pb.add("client_id", clientId); + if (clientSecret != null) pb.add("client_secret", clientSecret); + if (clientAssertionType != null) pb.add("client_assertion_type", clientAssertionType); + if (clientAssertion != null) pb.add("client_assertion", clientAssertion); + return pb.build(); + }Note: You’ll need to import URLParamUtil (and can optionally switch to using ParamNames constants via a static import for consistency).
86-94: Remove now-redundant helpers after switching to ParamBuilderIf you adopt the ParamBuilder approach above, these appendParam helpers become unused and can be deleted.
Apply this diff:
- private void appendParam(StringBuilder sb, String paramName, String paramValue) { - appendParam(sb, paramName, paramValue, e -> e); - } - - private void appendParam(StringBuilder sb, String paramName, String paramValue, Function<String, String> encoder) { - if (paramValue == null) - return; - sb.append("&").append(paramName).append("=").append(encoder.apply(paramValue)); - } + // removed: manual param appending; build() now uses URLParamUtil.ParamBuildercore/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/B2CMembrane.java (1)
155-164: Consider parameterizing auth method to keep coverage for client_secret_basic tooYou removed Basic header assertions on the server side. To prevent regressions, consider parameterizing this test fixture to run once with client_secret_basic and once with client_secret_post. That makes both paths observable in CI without duplicating scenarios.
I can provide a JUnit @ParameterizedTest variant or a small test matrix utility if you want to run the same suite against both auth modes.
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/MockAuthorizationServer.java (1)
165-174: Make client auth assertions robust to both Basic and POSTThe current assertions require client_id/client_secret in the body (client_secret_post). If future tests switch to client_secret_basic, this will fail silently for incorrect header handling. Prefer validating either Basic header or POST body depending on what’s present.
Apply this diff to support both methods:
- Map<String, String> params = URLParamUtil.getParams(new URIFactory(), exc, ERROR); - assertEquals(tc.clientId, params.get(CLIENT_ID)); - assertEquals(tc.clientSecret, params.get(CLIENT_SECRET)); + Map<String, String> params = URLParamUtil.getParams(new URIFactory(), exc, ERROR); + String authz = exc.getRequest().getHeader().getFirstValue("Authorization"); + if (authz != null && authz.startsWith("Basic ")) { + String decoded = new String(Base64.getDecoder().decode(authz.substring("Basic ".length())), UTF_8); + String[] parts = decoded.split(":", 2); + assertEquals(tc.clientId, parts[0], "client_id (basic)"); + assertEquals(tc.clientSecret, parts[1], "client_secret (basic)"); + } else { + assertEquals(tc.clientId, params.get(CLIENT_ID), "client_id (post)"); + assertEquals(tc.clientSecret, params.get(CLIENT_SECRET), "client_secret (post)"); + }core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java (3)
59-62: Tighten imports: add ISO_8859_1 and drop unused onesencode() and UTF_8 are not used here; ISO_8859_1 is needed for the Basic header fix.
Apply this diff:
-import static java.net.URLEncoder.encode; -import static java.nio.charset.StandardCharsets.UTF_8; +import static java.nio.charset.StandardCharsets.ISO_8859_1; import static org.apache.commons.codec.binary.Base64.encodeBase64;
48-48: Remove unused importURLEncoder is not used in this class.
Apply this diff:
-import java.net.URLEncoder;
233-247: Clarify precedence between clientAuthorization and useJWTForClientAuthConfig can currently set both useJWTForClientAuth=true and clientAuthorization=client_secret_*. The applyAuth refactor (above) makes JWT take precedence at runtime, but consider validating configuration during init() to fail fast or at least warn when both are configured.
Option (outside this hunk):
private void validateClientAuthConfig() { if (isUseJWTForClientAuth() && getClientSecret() != null) { log.warn("useJWTForClientAuth=true and clientSecret provided; JWT auth will be used and client_secret ignored."); } }Call from init() after properties are set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2TokenBody.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java(7 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/ClientAuthorization.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/B2CMembrane.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/MockAuthorizationServer.java(4 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/MockAuthorizationServer.java (2)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/ParamNames.java (1)
ParamNames(16-48)core/src/main/java/com/predic8/membrane/core/util/URLParamUtil.java (1)
URLParamUtil(31-159)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java (2)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2TokenBody.java (1)
OAuth2TokenBody(22-100)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/token/JWSSigner.java (1)
JWSSigner(26-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (9)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2TokenBody.java (2)
29-33: Good: client credentials and assertion fields added cohesivelyThe additional fields (clientId, clientSecret, clientAssertion, clientAssertionType) align with token_endpoint authentication options and the builder pattern used elsewhere.
34-34: Good: private ctor enforces builder usageMaking the constructor private nudges callers to the static builders and fluent setters.
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/B2CMembrane.java (1)
155-164: Explicitly exercising client_secret_post in tests is goodSetting client_secret_post here ensures tests validate the POSTed credentials path end-to-end.
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CTest.java (1)
509-521: Nice addition: verifies logout on error during the second flowThis complements multipleUserFlowsWithErrorTest by explicitly asserting the session logout state via /is-logged-in.
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/MockAuthorizationServer.java (3)
44-46: Good: replace magic strings with ParamNames constants and static ERRORThe static imports improve consistency and reduce typos across tests.
131-158: OK: consistent parameter parsing in one placeUsing URLParamUtil.getParams(..., ERROR) uniformly keeps parsing strict. No issues here.
192-193: Good: use SCOPE constantKeeps parameter handling aligned with the rest of the code and avoids string drift.
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java (2)
340-347: Good use of builder for authorization_code flowPassing redirectUri via the builder and delegating auth materialization to applyAuth is the right direction.
82-82: Sensible default for ClientAuthorizationDefaulting to client_secret_basic matches common provider defaults and keeps backward compatibility.
.../com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java
Show resolved
Hide resolved
.../com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java
Show resolved
Hide resolved
|
/ok-to-test |
2 participants
Summary by CodeRabbit