11:29 p.m.
Hi all,
I recently had to start working with Keycloak and, as I’m using a static code analyzer in
my IDE (Sonar Lint), I got lots of recommendations about Keycloak code.
These recommendations are not always very relevant but can help to increase the code
quality, the readability, can help to detect bugs (I actually found some) and this might
be an important point for the stability of the product, especially for a product managing
authentication.
First, in MultiValuedHashMap.equalsIgnoreValueOrder(), there is a bug but it can be
considered as assumed if we suppose that it is not possible to have duplicated values in
this map (which is often the case).
For a given key, if we compare associated lists of 2 MultivaluedHashMap instances [1, 2,
2] and [1, 2, 3], equalsIgnoreValueOrder will return true because lengths are equal and
all elements from the first list exist in the second list. Do you think it is necessary to
fix this ?
Static code analysis revealed issues described in the following examples and show some
inconsistencies : In FileTruststoreProviderFactory.init(), if pass is null, we “return” at
the beginning of the method… but pass is still compared to null twice after that.
Similary, in FreeMarkerUtil.processTemplate(), we set cache=null; then we compare cache to
null (if (cache!=null))
Don’t you think it could be better to follow some recommendations ? I could work on this :
as it should impact lots of code but maybe you should decide which type of recommendation
you want to follow and how you prefer to do this : all changes at once, one PR per type of
recommendation, on PR per maven module, …
Francis Pérot
(about me: I’m currently working on the multi-factor feature and we had a meeting two
weeks ago with Marek P. and Peter S. : the left ear you saw was mine 😊)
Example 1:
byte[] signature = null;
try {
signature =
HMACProvider.sign(buffer.toString().getBytes("UTF-8"), Algorithm.HS512,
sharedSecret);
} catch (UnsupportedEncodingException e) {
throw new RuntimeException(e);
}
Can become:
byte[] signature =
HMACProvider.sign(buffer.toString().getBytes(StandardCharsets.UTF_8), Algorithm.HS512,
sharedSecret);
Example 2:
LDAPObject ldapUser = ldapQuery.getFirstResult();
if (ldapUser == null) {
return null;
}
return ldapUser;
Can become:
return ldapQuery.getFirstResult();
Example 3:
if (responderURIs.size() == 0) {
if (responderURIs.isEmpty()) {
Example 4:
Set<Annotation> set = new HashSet<Annotation>();
Set<Annotation> set = new HashSet<>();
Other examples : https://github.com/fperot74/keycloak/pull/1/files