OA-42: Encode request parameter values in the logout url #30
Conversation
|
Empty query parameter is effectively a blank parameter value which is the same as any other parameter value in my opinion. |
| } | ||
|
|
||
| @Test | ||
| public void getPostLogoutRedirectUrl_shouldEncodeTheUrlIfDisabled() throws Exception { |
There was a problem hiding this comment.
Ooops! This should be shouldNotEncodeUrlIfDisabled
OA-42: Added property to disable encoding of post logout URL OA-42: Added property to disable encoding of post logout URL
| if (params != null) { | ||
| for (Map.Entry<String, List<String>> entry : params.entrySet()) { | ||
| final String encodedKey = URLEncoder.encode(entry.getKey()); | ||
| entry.getValue().forEach(v -> urlBuilderEncoded.queryParam(encodedKey, URLEncoder.encode(v))); | ||
| } | ||
| } |
There was a problem hiding this comment.
Empty query parameter is effectively a blank parameter value which is the same as any other parameter value in my opinion.
I get your point, I was suggesting to capture (or unit test) the above functionality. What is the expected outcome if params == null, can we capture that in tests
There was a problem hiding this comment.
I get what you are saying, but is there an IDP that accepts a parameterless logout URL anyways? I intentionally omitted that test, in fact, for this reason I wanted to remove that if clause instead, params can never be null or empty otherwise that would be an invalid URL since the client_id or the id_token_hint parameter is required but this could be Keycloak's implementation to require one of them, may be it is not the case for other implementations, so may be it make senses avoid a potential NPE in case of other IDPs that might might accept a parameterless logout URL.
There was a problem hiding this comment.
➕ it's a utility method, someone might want to use it to encode a URL that's not specific to Keycloak's implementation.
OA-42: Added property to disable encoding of post logout URL