Add WWW-Authenticate header.#47
Conversation
jimmyjames
left a comment
jimmyjames
left a comment
There was a problem hiding this comment.
Thanks for the PR! I think this is a good start towards adding the WWW-Authenticate header. Aside from the suggestions mentioned, we should also handle the case where the token is valid but has insufficient privileges (e.g., a 403). I think we'd need to implement the AccessDeniedHandler or possibly extend the AccessDeniedHandlerImpl, but I think it's fine for that to be separate from this PR unless you want to add it here.
| "WWW-Authenticate", | ||
| String.format("Bearer realm=\"%s\", authorization_uri=\"%soauth/token\"", this.audience, this.issuer) | ||
| ); | ||
|
|
There was a problem hiding this comment.
Let's use HttpHeaders.WWW-Authenticate instead of hard-coding the String literal.
For the header value, the authorization_uri you included here is really the token endpoint; the authorization uri would be /authorize. As the specification does not require the authorization_uri, I think it would be ok to not include it unless you have a use case you are thinking of.
There was a problem hiding this comment.
Hey Jimmy, thanks for your feedback.
The replacement of the headers key by the constant makes absolutely sense.
Regarding to the authorization_uri, the removal of this attribute would change the constructor to JwtAuthenticationEntryPoint(String audience). What do you think of keeping the current constructor JwtAuthenticationEntryPoint(String audience, String issuer) as a secondary one and making authorization_uri depending on the issuer so that authorization_uri is only written when the issuer is set?
I don't have such a deep understanding of the standard, as this is not my normal business. So feel free to reject my suggestions in case you see the standard violated here ;)
...and of course I will change the authroization_uri to [issuer]/authorize
There was a problem hiding this comment.
Making authroization_uri dependent on the issuer was a fallacy. I will provide another solution soon. thx.
…ization_uri's value
jimmyjames
left a comment
jimmyjames
left a comment
There was a problem hiding this comment.
Thanks for the update @meiXXI, and apologies for the delay on my end. I've added some more feedback. If you wish, we could also create an issue and I think we can try and take this on in the next week. Or, happy to continue to collaborate with you here on this PR. Thanks!
| final String audience; | ||
| final String issuer; | ||
|
|
||
| public JwtAuthenticationEntryPoint(String audience, String issuer) { |
There was a problem hiding this comment.
We will need to provide a default no-arg constructor, to ensure that existing usages of this class aren't broken with this change.
|
|
||
| @Override | ||
| public void commence(HttpServletRequest request, HttpServletResponse response, AuthenticationException authException) throws IOException, ServletException { | ||
| String modifiedIssuer = this.issuer.endsWith("/") ? this.issuer.substring(0, this.issuer.length() - 1) : this.issuer; |
There was a problem hiding this comment.
Is this to ensure that the header value won't have double "/"? We will need to account for potential null values (related to the above comment regarding needing a default constructor, in which audience and issuer would be null). Also would be good to make sure all cases are covered by tests, that might be why the code coverage check is failing.
|
I've created #50 to track the issue. I'll follow with a separate PR shortly based on the conversation here to make sure we get this addressed. |
|
Fixed with #51 |
2 participants
This change adds the WWW-Authenticate header in case of a 401 response, as the JWT standard requires it.