Re: [keycloak-dev] Code quality & bugs

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(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/keycloak-dev Regards, Tomas _______________________________________________ keycloak-dev mailing list keycloak-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/keycloak-dev