store the B2C flow info in the OAuth2 state and attach it to the request token #2046
Conversation
WalkthroughThis change introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OAuth2Resource2Interceptor
participant FlowInitiator
participant StateManager
participant AuthorizationService
participant MockAuthorizationServer
Client->>OAuth2Resource2Interceptor: Initiate OAuth2 flow
OAuth2Resource2Interceptor->>FlowInitiator: handleRequestInternal()
FlowInitiator->>AuthorizationService: respondWithRedirect(exc, FlowContext)
AuthorizationService->>StateManager: buildStateParameter(..., FlowContext)
StateManager-->>AuthorizationService: state parameter with FlowContext
AuthorizationService-->>FlowInitiator: Redirect response with FlowContext in state
FlowInitiator-->>Client: Redirect to Auth Server
Client->>MockAuthorizationServer: Authorization request (with state)
MockAuthorizationServer-->>Client: Redirect with code (flow-specific)
Client->>OAuth2Resource2Interceptor: Callback with code & state
OAuth2Resource2Interceptor->>StateManager: Parse state, extract FlowContext
OAuth2Resource2Interceptor->>AuthorizationService: codeTokenRequest(..., FlowContext)
AuthorizationService->>MockAuthorizationServer: Token request (flow-specific endpoint)
MockAuthorizationServer-->>AuthorizationService: Token response (flow-specific)
AuthorizationService-->>OAuth2Resource2Interceptor: Token & FlowContext
OAuth2Resource2Interceptor-->>Client: Final response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
/ok-to-test |
|
This pull request needs "/ok-to-test" from an authorized committer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
core/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.java (1)
666-668: Consider whether the new default (10 KB) is still safe for production logsThe default limit has jumped from 1 KB to 10 KB.
While useful for debugging, logging larger bodies increases log volume and the risk of leaking sensitive data.
You may want to keep the previous default (1–2 KB) and rely on the system property for opt-in expansion.core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/FlowContext.java (1)
71-74: Consider using a more robust URL encoding approach.The manual URL encoding with hardcoded percent-encoded characters (
%26fcd%3D) is fragile and could lead to double-encoding issues.Consider using a builder pattern or proper URL parameter encoding:
- return fc != null ? "%26fcd%3D" + urlencode(fc.defaultFlow) + "%26fct%3D" + urlencode(fc.triggerFlow) : ""; + return fc != null ? "&fcd=" + urlencode(fc.defaultFlow) + "&fct=" + urlencode(fc.triggerFlow) : "";Note: If the pre-encoded format is required for compatibility, add a comment explaining why.
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/FlowInitiator.java (1)
124-126: Make the flow replacement pattern more specific.The current regex pattern could inadvertently replace flow names in query parameters or other parts of the URL.
Consider using a more specific pattern that only matches path segments:
private String replaceFlow(String text) { - return text.replaceAll("/" + defaultFlow + "/", "/" + triggerFlow + "/"); + return text.replaceAll("/" + Pattern.quote(defaultFlow) + "/", "/" + triggerFlow + "/"); }Also add import:
import java.util.regex.Pattern;core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java (1)
315-323: Consider adding null check for FlowContext parameter.While the implementation correctly uses FlowContext, consider adding a null check or documenting whether null is an acceptable value, especially since
getTokenEndpointhandles null FlowContext.public OAuth2TokenResponseBody codeTokenRequest(String redirectUri, String code, String verifier, FlowContext flowContext) throws Exception { + // flowContext can be null for non-B2C flows return parseTokenResponse(checkTokenResponse(doRequest(applyAuth(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java(6 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/FlowContext.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/FlowInitiator.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/OAuth2Resource2Interceptor.java(3 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/OAuth2CallbackRequestHandler.java(3 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/StateManager.java(5 hunks)core/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.java(2 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/MockAuthorizationServer.java(6 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CTest.java(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java (1)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/FlowContext.java (1)
FlowContext(31-102)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/FlowInitiator.java (1)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/FlowContext.java (1)
FlowContext(31-102)
⏰ 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)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (11)
core/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.java (1)
52-53: Configurable trace-body length is a welcome additionReplacing the hard-coded 1 KB limit with a configurable constant improves flexibility and debuggability.
No issues spotted in this declaration itself.core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/OAuth2CallbackRequestHandler.java (1)
32-32: LGTM! Flow context integration is correct.The changes properly propagate the flow context from the OAuth2 state parameter through the token request and into the session, which aligns with the PR objectives to store B2C flow info in the OAuth2 state.
Also applies to: 107-107, 125-125
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/OAuth2Resource2Interceptor.java (1)
142-142: LGTM! FlowContext parameter properly added to redirect handling.The method signature change and subsequent usage correctly propagate the flow context through the OAuth2 redirect flow, ensuring the B2C flow information is included in the state parameter.
Also applies to: 169-169, 252-252, 260-260
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/MockAuthorizationServer.java (1)
49-49: LGTM! Test server correctly simulates flow-specific OAuth2 behavior.The mock authorization server properly tracks refresh tokens per flow and validates them accordingly. The use of
ConcurrentHashMapensures thread safety for the refresh token storage.Also applies to: 68-69, 143-146, 166-174, 200-204
core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/client/b2c/OAuth2ResourceB2CTest.java (2)
528-534: Good security enhancement with audience validation.Adding expected audience validation to the JWT consumer strengthens token security by ensuring tokens are only accepted by their intended recipients. This is particularly important in B2C scenarios with multiple flows.
336-362: Excellent test coverage for flow context edge cases.This test effectively validates the critical scenario where users start but abort a new flow, ensuring the system correctly maintains flow context for refresh tokens. The assertions properly verify that tokens are issued from the expected flows.
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/rf/StateManager.java (3)
48-60: Well-structured FlowContext integration.The FlowContext field is properly integrated into both constructors, maintaining consistency between creation and deserialization paths. The delegation to
FlowContext.fromUrlParamfor deserialization maintains good separation of concerns.
68-75: Public visibility change is justified for code reuse.Making
getValueFromStatepublic enablesFlowContextto reuse this state parsing logic, which is appropriate. The method properly handles null checks and error cases.
122-125: FlowContext.toUrlParam correctly URL-encodes its valuesVerified that
FlowContext.toUrlParam(…)invokesOAuth2Util.urlencode, which usesURLEncoder.encode(..., UTF_8)(with “+” replaced by “%20”). BothdefaultFlowandtriggerFloware fully percent-encoded, and the surrounding “%26”/“%3D” literals safely encode the ampersand and equals. No further changes needed.core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java (2)
231-242: Proper FlowContext propagation to client authentication.The FlowContext is correctly passed through to
createClientToken, ensuring JWT client assertions have the appropriate audience for the specific B2C flow.
304-313: Correct flow context handling in refresh token requests.The implementation properly extracts FlowContext from the session and uses it consistently for both the token endpoint URL and authentication, maintaining flow context through token refreshes.
.../com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java
Show resolved
Hide resolved
...main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/FlowContext.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2client/FlowInitiator.java
Show resolved
Hide resolved
core/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.java
Show resolved
Hide resolved
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests