[keycloak-dev] Code quality & bugs

[Tomas Kyjovsky]

Hi Francis,

See my comments inline..

----- Original Message -----
> 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 ?

I created JIRA and posted PR for this: https://github.com/keycloak/keycloak/pull/6544

> 
> 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

I had a brief look at these changes and I agree, this would clean up the code and improve readability.
It's a lot of changes though (445 files) which might take a long to review and I'm not sure about the priority of this 
compared to some other tasks like wrapping up the multi-factor stuff (KEYCLOAK-7159). I'll discuss this with colleagues and get back to you.

> _______________________________________________
> keycloak-dev mailing list
> keycloak-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/keycloak-dev


Regards,
Tomas