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:
[email protected]
