smolnar82 commented on code in PR #949: URL: https://github.com/apache/knox/pull/949#discussion_r1830444857
########## gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java: ########## @@ -512,17 +520,22 @@ protected boolean verifyTokenSignature(final JWT token) { // If it has not yet been verified, then perform the verification now if (!verified) { try { + boolean hasPem = false; + boolean hasJWKS = false; + if (publicKey != null) { + hasPem = true; verified = authority.verifyToken(token, publicKey); log.pemVerificationResultMessage(verified); } if (!verified && expectedJWKSUrls != null && !expectedJWKSUrls.isEmpty()) { + hasJWKS = true; verified = authority.verifyToken(token, expectedJWKSUrls, expectedSigAlg, allowedJwsTypes); log.jwksVerificationResultMessage(verified); } - if(!verified) { + if(!verified && ((!hasPem && !hasJWKS) || isJwtInstanceKeyFallback)) { Review Comment: I think `hasPEM` and `hasJWKS` are redundant (or need a better name). First, `hasJWKS` is misleading, because it's only set to `true` when - no PEM configured - PEM configured, but the PEM verification attempt failed So even if JWKS is configured, it might remain `false`. If I were you, I'd only rely on the already existing class members as follows: - new private method: `private boolean configuredPEM() { return publicKey != null; }` - new private method `private boolean configuredJWKS() { return expectedJWKSUrls != null && !expectedJWKSUrls.isEmpty(); }` - then, you could use these new methods in lines 526 and 532 as well as in the new condition in line 538. Even better, I'd create another new private method for that purpose: ``` private boolean verifyInstanceKeys() { //name might be changing return isJwtInstanceKeyFallback || (!hasPEM() && !hasJWKS()); } ``` So the updated condition would look like this: `if (!verified && verifyInstanceKeys()) {` Thus, the new boolean variables can be removed. -- 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. To unsubscribe, e-mail: dev-unsubscr...@knox.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org