[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token

[GitBox <gi...@apache.org>]

mike-jumper commented on a change in pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#discussion_r524993693



##########
File path: extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java
##########
@@ -88,12 +89,12 @@ public String processUsername(String token) throws GuacamoleException {
                 .setVerificationKeyResolver(resolver)
                 .build();
 
-        try {
+        JwtClaims claims = null;

Review comment:
       The logic:
   
   ```java
   JwtClaims claims = null;
   try {
       claims = somethingElse;
       doProcessing(claims);
       doMoreProcessing(claims);
   }
   catch (VariousExceptions e) {
      logger.debug("Yikes.", e);
   }
   
   return claims;
   ```
   
   is dangerous because `claims` might become non-null yet still fail processing. This function would then effectively pretend to have succeeded, authenticating a user that is not actually properly authenticated.
   
   Consider:
   
   1. `claims` is initialized to `null`
   2. `claims` is set to the result of `jwtConsumer.processToClaims(token)`
   3. The call to `claims.getStringClaimValue()` fails with `MalformedClaimException`
   4. Details of the exception are logged, but code just falls through to `return claims`, incorrectly returning a non-null value.
   
   Even if the logic can be massaged to avoid this, or it can be shown that the above specific circumstance cannot occur, it's dangerous to maintain this form as it would be too easy for future changes in this area to introduce such a problem.
   
   I recommend adopting the same form as used below in `processUsername()` - returning `claims` _only_ at the end of the `try`, with the final part of the function being the `return null` for what is then known to be a pure failure case.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org