pzampino commented on code in PR #949: URL: https://github.com/apache/knox/pull/949#discussion_r1833401823
########## 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: The booleans are intended to indicate which methods have been attempted, and they avoid additional (albeit minimal) method overhead; They're effectively caching the result of the evaluations you're proposing to repeat. I do see your point about hasJWKS being a little misleading, but I'm not convinced it matters since we only really care if they're BOTH (PEM, JWKS) missing. I could be persuaded to rename them to something like attemptedPEMVerification and attemptedJwksVerification, which more accurately reflect their respective intentions. What do you think? -- 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