Deprecate ResponseMode.QUERY in system browser auth flow, automatically override to FORM_POST with warning #1004
Conversation
…ly override to FORM_POST with warning
There was a problem hiding this comment.
Pull request overview
This pull request deprecates the ResponseMode.QUERY option in the system browser authentication flow due to lack of support. When developers pass ResponseMode.QUERY, it is now automatically overridden to ResponseMode.FORM_POST with a warning log message, ensuring backward compatibility.
Changes:
- Added deprecation annotation to
ResponseMode.QUERYenum value with documentation explaining the override behavior - Implemented automatic override logic in
AuthorizationRequestUrlParametersto convert QUERY to FORM_POST with a warning log - Replaced comprehensive optional parameters test with a focused test for QUERY override behavior
- Added build artifact (log4j.properties) to version control
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ResponseMode.java | Added deprecation annotation and documentation to QUERY enum value |
| msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/AuthorizationRequestUrlParameters.java | Implemented override logic to convert QUERY to FORM_POST with warning, added deprecation to responseMode() method |
| msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/AuthorizationRequestUrlParametersTest.java | Replaced comprehensive optional parameters test with focused QUERY override validation test |
| msal4j-persistence-extension/target/test-classes/log4j.properties | Added log4j configuration file in build output directory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log4j.rootLogger=TRACE, stdout | ||
| log4j.appender.stdout=org.apache.log4j.ConsoleAppender | ||
| log4j.appender.stdout.Target=System.out | ||
| log4j.appender.stdout.layout=org.apache.log4j.PatternLayout | ||
| log4j.appender.stdout.layout.ConversionPattern=%d{yyyy-MM-dd'T'HH:mm:ss.SSS} %-5p [%c] - %m%n No newline at end of file |
There was a problem hiding this comment.
The log4j.properties file in the target directory should not be included in version control. The 'target' directory is typically a build output directory (Maven/Gradle) and should be excluded from version control. This file appears to be a build artifact that was accidentally committed.
| log4j.rootLogger=TRACE, stdout | |
| log4j.appender.stdout=org.apache.log4j.ConsoleAppender | |
| log4j.appender.stdout.Target=System.out | |
| log4j.appender.stdout.layout=org.apache.log4j.PatternLayout | |
| log4j.appender.stdout.layout.ConversionPattern=%d{yyyy-MM-dd'T'HH:mm:ss.SSS} %-5p [%c] - %m%n | |
| # This file is located under the Maven/Gradle build output directory: | |
| # msal4j-persistence-extension/target/test-classes/log4j.properties | |
| # It should not be tracked in version control and should be removed | |
| # from the repository, with the entire `target/` directory ignored | |
| # via .gitignore (or equivalent). | |
| # | |
| # The contents below were removed to prevent this accidental artifact | |
| # from affecting runtime logging configuration. Do not add any active | |
| # Log4j configuration here; instead, place it under src/test/resources | |
| # or src/main/resources as appropriate. |
There was a problem hiding this comment.
I agree with Copilot here, looks like this was accidentally committed?
| * Specifies the method that should be used to send the authentication result to your app. | ||
| * @deprecated ResponseMode.QUERY is deprecated. If you pass ResponseMode.QUERY, it will be automatically overridden to ResponseMode.FORM_POST. | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
Marking the responseMode() method as deprecated is confusing and incorrect. Only ResponseMode.QUERY is deprecated, but ResponseMode.FORM_POST and ResponseMode.FRAGMENT remain valid. The @deprecated annotation should be removed from the method itself. The method should remain non-deprecated so developers can still use it with the supported response modes. The deprecation should only be on the ResponseMode.QUERY enum value itself, which is already done in ResponseMode.java.
| @Deprecated |
There was a problem hiding this comment.
I agree with Copilot here, the comment is helpful but having @Deprecated here implies the method itself will be fully removed in the future.
| } | ||
|
|
||
| @Test | ||
| void testBuilder_optionalParameters() throws UnsupportedEncodingException { |
There was a problem hiding this comment.
Why rename the test and remove most of what it was checking?
In the existing test there already was an assert for assertEquals(queryParameters.get("response_mode"), "query");, you should be able to just have that look for "form_post" or Response.Mode.FORM_POST instead
| this.responseMode = builder.responseMode; | ||
| requestParameters.put("response_mode", | ||
| builder.responseMode.toString()); | ||
| // Override QUERY with FORM_POST as QUERY is deprecated |
There was a problem hiding this comment.
Rather than add the logic to check for Query here, it might be simpler to add an if statement to the actual builder method:
public Builder responseMode(ResponseMode val) {
// Override QUERY with FORM_POST as QUERY is deprecated
if (val == ResponseMode.QUERY) {
LOG.warn("ResponseMode.QUERY is deprecated and will be removed in a future release. " +
"Automatically overriding to ResponseMode.FORM_POST.");
this.responseMode = ResponseMode.FORM_POST;
} else {
this.responseMode = val;
}
return self();
}
That way, this constructor stays the same and only valid values get set in the builder.
Avery-Dunn
left a comment
Avery-Dunn
left a comment
There was a problem hiding this comment.
See above comments: looks like there are some unnecessary additions and changes to the tests.
The system browser authentication flow no longer supports query response mode. This change deprecates ResponseMode.QUERY . When developers pass ResponseMode.QUERY, it will be automatically overridden to FORM_POST with a warning log, ensuring backward compatibility while guiding users to the supported mode.