Specify charset in WWW-Authenticate for Basic Auth#18760
Specify charset in WWW-Authenticate for Basic Auth#18760therepanic wants to merge 1 commit intospring-projects:mainfrom
WWW-Authenticate for Basic Auth#18760Conversation
| this.credentialsCharset = credentialsCharset; | ||
| if (this.authenticationConverter instanceof BasicAuthenticationConverter basicAuthenticationConverter) { | ||
| basicAuthenticationConverter.setCredentialsCharset(Charset.forName(credentialsCharset)); | ||
| Charset charset = Charset.forName(credentialsCharset); | ||
| basicAuthenticationConverter.setCredentialsCharset(charset); | ||
| if (this.authenticationEntryPoint instanceof BasicAuthenticationEntryPoint basicAuthenticationEntryPoint) { | ||
| basicAuthenticationEntryPoint.setCharset(charset); | ||
| } | ||
| } |
There was a problem hiding this comment.
BTW, I think this is a cool solution for charset forwarding, just my 50 cents.
spring-projects-issues
added
the
status: waiting-for-triage
| @@ -53,7 +58,8 @@ public void afterPropertiesSet() { | |||
| @Override | |||
| public void commence(HttpServletRequest request, HttpServletResponse response, | |||
| AuthenticationException authException) throws IOException { | |||
| response.setHeader("WWW-Authenticate", "Basic realm=\"" + this.realmName + "\""); | |||
| String header = "Basic realm=\"" + this.realmName + "\", charset=\"" + this.charset.name() + "\""; | |||
| response.setHeader("WWW-Authenticate", header); | |||
There was a problem hiding this comment.
RFC-7617 states that charset is recommended but not mandatory.
This essentially leads us to different conclusions. Ideally, we should make it nullable to comply with this RFC. However, on the other hand, I see no reason to go with this convention. In what I believe is an elegant solution I've presented, unfortunately, we can't pass a nullable charset because BasicAuthenticationFilter#setCredentialsCharset doesn't accept a nullable value, and I certainly don't think we should change the contract for this reason. I'd like to hear other opinions on this matter, in case anyone needs it.
| assertThat(headers.get(0)).isEqualTo("Basic realm=\"hello\""); | ||
| assertThat(headers.get(0)).isEqualTo("Basic realm=\"hello\", charset=\"UTF-8\""); |
There was a problem hiding this comment.
I believe this isn't a breaking change. Clients will ignore the header parameters, and this is just a convention of our test.
In this commit, we add support for the charset from RFC-7617, which definitely solves the problem when the client does not know what charset we are parsing with. Closes: spring-projectsgh-18755 Signed-off-by: Andrey Litvitski <andrey1010102008@gmail.com>
|
It turned out that some tests also required modifications and the addition of a charset. I wrote about this here: #18760 (comment) and here: #18760 (comment). And over time, I'm starting to think that maybe the tests should still pass in this case? Then we'll have to create some kind of toggle switch, so we can enable charset additions. I'd like to know the best way to do this to fit the project. I'm not sure what's best. |
2 participants
In this commit, we add support for the charset from RFC-7617, which definitely solves the problem when the client does not know what charset we are parsing with.
Closes: gh-18755