[keycloak-dev] Code quality & bugs

[Marek Posolda]

On 25. 11. 19 21:29, Tomas Kyjovsky wrote:
> 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);
+1 to fix, but I am not sure if it is not fixed already in the meantime 
in latest Keycloak master? I recall seeing some recent PR related to 
this. It can be related to quarkus related work.
>>
>> Example 2:
>>              LDAPObject ldapUser = ldapQuery.getFirstResult();
>>              if (ldapUser == null) {
>>                  return null;
>>              }
>>
>>              return ldapUser;
>> Can become:
>>              return ldapQuery.getFirstResult();
+1 to fix. BTV. It's quite possible I wrote this non-sense code :)
>>
>> Example 3:
>>
>>
>>          if (responderURIs.size() == 0) {
>>
>>
>>          if (responderURIs.isEmpty()) {
+1 to fix.
>>
>>
>>
>> Example 4:
>>
>>
>>          Set<Annotation> set = new HashSet<Annotation>();
>>
>>
>>          Set<Annotation> set = new HashSet<>();

This is bit tricky. AFAIK this can be done just on Java 8? But some of 
the adapter modules still rely on JDK 7 and I think we still want them 
to be "compilable" on JDK 7 as well.

If it's the case, it may be good to either post-pone this optimization 
or do it just in the java modules, which can run only on server 
(server-spi, server-spi-private, services, some federation modules etc)

Marek

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

 From very brief look, there are lots of changes for example 4 in the 
adapter module. Not sure if those can be done or not. I vote for not ATM 
unless you're sure that this stuff works on all JDK 7 versions.

Marek

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