Membrane Auth Sever: Configurable Response Modes#2013
Conversation
WalkthroughThis set of changes refactors string parsing and normalization for CORS and OAuth2 modules, introduces new utility classes ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MembraneAuthorizationService
participant OAuth2Server
Client->>MembraneAuthorizationService: Request login URL
MembraneAuthorizationService->>OAuth2Server: Fetch .well-known/openid-configuration
OAuth2Server-->>MembraneAuthorizationService: Return JSON config (response_modes_supported)
MembraneAuthorizationService->>MembraneAuthorizationService: Parse and negotiate response mode
MembraneAuthorizationService-->>Client: Return login URL with negotiated response_mode
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
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: 2
🧹 Nitpick comments (3)
core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (1)
35-59: Consider removing commented codeThe commented
parseCommaOrSpaceSeparatedmethod should be removed since it's been superseded by theStringListutility class. Commented code can create confusion and maintenance overhead.-// /** -// * Parses a string into a list of trimmed, non-empty string tokens. -// * -// * <p>This method supports two parsing modes:</p> -// * <ul> -// * <li><strong>Comma-separated:</strong> Standard HTTP header format (e.g., "GET, POST, PUT")</li> -// * <li><strong>Space-separated:</strong> Alternative format (e.g., "GET POST PUT")</li> -// * </ul> -// * -// * <p>The method automatically trims whitespace from each token and excludes empty values, -// * making it robust against various formatting inconsistencies.</p> -// * -// * @param value the string to parse. Can be null or empty. -// * @return a non-null list of parsed tokens. Returns an empty list if input is null/empty. -// -// * @apiNote This dual parsing behavior is intended for flexibility in configuration formats. -// * For strict HTTP header compliance, consider using comma-only separation. -// * @since 6.2.0 -// */ -// public static @NotNull Set<String> parseCommaOrSpaceSeparated(String value) { -// return stream(value.split("\\s*,\\s*|\\s+")) -// .map(String::trim) -// .filter(s -> !s.isEmpty()) -// .collect(toSet()); -// }core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/MembraneAuthorizationService.java (2)
152-173: Excellent JSON parsing refactoring with a minor typo.The switch to
JsonNodewithpath().asText()provides safer field extraction and better null handling. The added logging is helpful for debugging.Fix the typo in the log message:
- log.debug("Aggreed on response mode: {}", responseMode); + log.debug("Agreed on response mode: {}", responseMode);
175-177: Add null check for defensive programming.The method should handle null input gracefully.
private static List<String> convertToListOfStrings(ObjectMapper mapper, JsonNode json) { + if (json == null || json.isNull()) { + return null; + } return mapper.convertValue(json, new TypeReference<>() {}); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
annot/src/main/java/com/predic8/membrane/annot/MCAttribute.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/cors/CorsInterceptor.java(3 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/cors/CorsUtil.java(0 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/cors/PreflightHandler.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/MembraneAuthorizationService.java(8 hunks)core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java(2 hunks)core/src/main/java/com/predic8/membrane/core/util/StringList.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/cors/CorsInterceptorTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/MembraneAuthorizationServiceTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/util/CollectionsUtilTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/util/StringListTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/cors/CorsUtil.java
🔇 Additional comments (26)
annot/src/main/java/com/predic8/membrane/annot/MCAttribute.java (3)
16-19: LGTM! Clean import consolidation.The wildcard import for annotation classes and static imports for frequently used constants improve readability without affecting functionality.
24-25: LGTM! Proper use of static imports.Using static imports for
METHODandRUNTIMEmakes the annotation declarations more concise and readable.
28-28: LGTM! Redundant modifier removed.Correctly removed the redundant
publicmodifier since interface methods are implicitly public.core/src/test/java/com/predic8/membrane/core/util/CollectionsUtilTest.java (1)
46-46: LGTM! Consistent assertion style.Using the static import for
assertIterableEqualsis consistent with other assertions in the file and improves readability.core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/AuthorizationService.java (1)
58-62: router field visibility change is safeA search for subclasses of
AuthorizationServiceand any direct accesses toroutershows:
- No classes outside
com.predic8.membrane.core.interceptor.oauth2.authorizationserviceextend or reference therouterfield.- The only direct assignment in
MembraneAuthorizationServiceTest.javaresides in the same package.Changing
routerfromprotectedto package-private does not introduce breaking changes.core/src/test/java/com/predic8/membrane/core/interceptor/cors/CorsInterceptorTest.java (1)
480-480: LGTM! Proper adaptation to utility refactoring.The explicit call to
CollectionsUtil.toLowerCaseSetcorrectly reflects the refactoring where utility methods were moved fromCorsUtiltoCollectionsUtil.core/src/main/java/com/predic8/membrane/core/interceptor/cors/PreflightHandler.java (2)
25-29: LGTM! Proper import updates for utility refactoring.The import changes correctly reflect the refactoring where parsing utilities were moved to
StringListandCollectionsUtil. The wildcard import forResponseHeaderBuilderis appropriate given multiple members are used.
92-92: PreflightHandler update is safe:parseToSetmatches prior behaviorThe
parseToSetshortcut delegates to:parse(value, LinkedHashSet::new)and
parseitself usesvalue.split("\\s*,\\s*|\\s+")which covers both comma- and whitespace-separation, identical to the old
parseCommaOrSpaceSeparatedlogic. Existing tests inStringListTestconfirm deduplication and correct splitting. No further changes are needed.core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (2)
19-19: LGTM: Clean static import additionThe static import for
Collectorsimproves code readability in thetoLowerCaseSetmethod.
61-71: LGTM: Correct implementation of case normalization utilityThe
toLowerCaseSetmethod correctly converts all strings to lowercase and returns a new set, maintaining immutability of the input set.core/src/main/java/com/predic8/membrane/core/interceptor/cors/CorsInterceptor.java (3)
241-241: LGTM: Consistent parsing logic migrationThe migration to
StringList.parseToSetmaintains the same parsing behavior while using the centralized utility.
266-266: LGTM: Correct header normalizationThe combination of
StringList.parseToSetandCollectionsUtil.toLowerCaseSetproperly handles header parsing and case normalization, which is appropriate since HTTP headers are case-insensitive.
291-291: LGTM: Consistent header processingSame correct pattern as the
setHeadersmethod - parsing followed by case normalization for HTTP headers.core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/MembraneAuthorizationServiceTest.java (1)
19-106: LGTM: Comprehensive test coverageThe test class provides good coverage of response mode negotiation scenarios, including edge cases like empty lists and no matches. The mock setup is appropriate for testing the service in isolation.
core/src/test/java/com/predic8/membrane/core/util/StringListTest.java (1)
15-71: LGTM: Comprehensive test coverageThe test class provides excellent coverage of the
StringListutility, including:
- Edge cases (null, blank input)
- Different separator types (comma, space, mixed)
- Collection type variations (list, set, custom)
- Deduplication behavior for sets
The nested test structure improves readability and organization.
core/src/main/java/com/predic8/membrane/core/util/StringList.java (3)
13-15: LGTM: Proper utility class designThe final class with private constructor follows the standard utility class pattern, preventing instantiation and inheritance.
34-41: LGTM: Robust parsing implementationThe implementation correctly handles:
- Null input (converted to empty string)
- Regex pattern
\\s*,\\s*|\\s+properly matches comma-separated OR space-separated values- Trimming and filtering empty strings prevents malformed tokens
- Generic collection factory provides flexibility
44-51: LGTM: Well-designed convenience methodsBoth convenience methods provide appropriate defaults:
parseToListreturnsArrayListfor general use casesparseToSetreturnsLinkedHashSetto preserve insertion order while eliminating duplicatesThe choice of
LinkedHashSetoverHashSetis particularly good for maintaining predictable iteration order.core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/MembraneAuthorizationService.java (8)
16-30: LGTM! Well-organized imports.The consolidated imports properly group related classes and include necessary dependencies for the new functionality.
34-45: Well-structured constants for response mode configuration.Good design choices:
- Proper use of constants for response modes
- Smart fallback strategy excluding
form_postwhich requires server-side support- Clear documentation and appropriate use of
@NotNullannotation
63-69: Appropriate field declarations for response mode support.The default priority order (form_post, query, fragment) aligns with security best practices, as form_post provides better protection against token leakage.
93-95: Good refactoring to use constant.Using the constant improves maintainability and consistency.
109-116: Clean method signatures.Removing unnecessary exception declarations improves the API clarity since these methods don't perform operations that throw checked exceptions.
Also applies to: 143-150
179-189: Well-implemented response mode negotiation.Excellent implementation with:
- Proper fallback handling for servers that don't advertise response modes
- Clear priority-based selection
- Informative error message for configuration issues
- Package-private visibility for testability
200-211: Simplified and correct response mode handling.Good improvement - always including the negotiated response mode makes the behavior more predictable and spec-compliant.
303-312: Excellent configuration method with clear documentation.The JavaDoc provides a helpful example and clearly explains the preference ordering. The MCAttribute annotation properly integrates with Membrane's configuration system.
…2/authorizationservice/MembraneAuthorizationServiceTest.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…2/authorizationservice/MembraneAuthorizationServiceTest.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
… membrane-as-response-modes
|
/ok-to-test |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/MembraneAuthorizationService.java (2)
109-116: Minor: Consider adding IOException to method signatures.The removal of declared exceptions from method signatures is fine if the methods truly don't throw checked exceptions, but ensure this doesn't hide potential issues.
314-316: Consider adding setter for responseMode.The getter for
responseModeis exposed but there's no corresponding setter. Consider whether this field should be configurable or if it should remain read-only as a result of negotiation.If
responseModeshould remain read-only (recommended), consider adding documentation to clarify this:+/** + * @return The negotiated response mode determined during initialization. + * This value is read-only and determined by negotiating between configured + * preferences and server capabilities. + */ public String getResponseMode() { return responseMode; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/src/main/java/com/predic8/membrane/core/interceptor/cors/AbstractCORSHandler.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/cors/CorsInterceptor.java(3 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/cors/CorsUtil.java(0 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/cors/PreflightHandler.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/MembraneAuthorizationService.java(8 hunks)core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java(2 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/MembraneAuthorizationServiceTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/cors/CorsUtil.java
✅ Files skipped from review due to trivial changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/cors/AbstractCORSHandler.java
🚧 Files skipped from review as they are similar to previous changes (4)
- core/src/main/java/com/predic8/membrane/core/interceptor/cors/PreflightHandler.java
- core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java
- core/src/test/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/MembraneAuthorizationServiceTest.java
- core/src/main/java/com/predic8/membrane/core/interceptor/cors/CorsInterceptor.java
⏰ 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 (9)
core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/authorizationservice/MembraneAuthorizationService.java (9)
16-26: LGTM! Clean import organization.The new imports are properly organized and necessary for the implemented functionality including Jackson for JSON processing, annotations for configuration, and logging.
34-44: Well-structured constants and defaults.The constants for response modes and default fallback list are well-defined. The use of
@NotNullannotations on the list elements is a good practice for null safety.
63-68: Good design with configurable priority order.The
responseModesSupportedfield with default priority order (form_post > query > fragment) follows security best practices by preferring form_post, which is more secure than query parameters for sensitive data.
94-94: Use constant for consistency.The well-known configuration path construction now uses the defined constant, which improves maintainability.
143-150: Method visibility change is appropriate.Making
adjustScope()private is good encapsulation since it's only used internally.
154-173: Robust JSON parsing with proper error handling.The refactoring to use Jackson's
JsonNodewithpath().asText()is safer than direct field access and provides better null handling. The logging for response mode negotiation provides good visibility into the process.
179-188: Solid negotiation logic with proper error handling.The response mode negotiation follows a clear priority-based selection algorithm. The error message in the
ConfigurationExceptionprovides good debugging information. The logic correctly handles the case where the server offers no response modes by falling back to defaults.
299-312: Well-designed configuration API with proper documentation.The getter/setter pair for
responseModesSupportedis well-implemented:
- The getter uses
CollectionsUtil.join()for consistent string representation- The setter uses
StringList.parseToList()for flexible input parsing- Good documentation with examples and default values
- Proper
@MCAttributeannotation for configuration binding
209-209: No risk of null responseMode in getLoginURL–
responseModeis set unconditionally ininit()vianegotiateResponseMode(...), which falls back to defaults and throws if nothing matches.
– All callers (e.g. the OAuth2 interceptors) invokeinit()before anygetLoginURL()call.
– Tests cover both default and custom modes, provingresponseModeis never null.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests