psykibaek commented on code in PR #21518:
URL: https://github.com/apache/kafka/pull/21518#discussion_r2878002623
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/DefaultJwtValidator.java:
##########
@@ -33,33 +32,43 @@
/**
* This {@link JwtValidator} uses the delegation approach, instantiating and
delegating calls to a
- * more concrete implementation. The underlying implementation is determined
by the presence/absence
- * of the {@link VerificationKeyResolver}: if it's present, a {@link
BrokerJwtValidator} is
- * created, otherwise a {@link ClientJwtValidator} is created.
+ * more concrete implementation. The underlying implementation is determined
by the configuration:
+ * if a JWKS endpoint URL is configured or a verification key resolver is
provided,
+ * a {@link BrokerJwtValidator} is created, otherwise a {@link
ClientJwtValidator} is created.
+ *
+ * <p>Note: {@link BrokerJwtValidator} and its jose4j dependency are loaded
lazily via reflection
+ * to avoid {@link ClassNotFoundException} in client-only environments where
jose4j is not
+ * on the classpath.
*/
public class DefaultJwtValidator implements JwtValidator {
- private final Optional<CloseableVerificationKeyResolver>
verificationKeyResolver;
+ private static final String BROKER_JWT_VALIDATOR_CLASS =
+ "org.apache.kafka.common.security.oauthbearer.BrokerJwtValidator";
+
+ private static final String CLOSEABLE_VERIFICATION_KEY_RESOLVER_CLASS =
+
"org.apache.kafka.common.security.oauthbearer.internals.secured.CloseableVerificationKeyResolver";
+
+ private final Optional<Object> verificationKeyResolver;
private JwtValidator delegate;
public DefaultJwtValidator() {
this.verificationKeyResolver = Optional.empty();
}
- public DefaultJwtValidator(CloseableVerificationKeyResolver
verificationKeyResolver) {
+ public DefaultJwtValidator(Object verificationKeyResolver) {
Review Comment:
@kirktrue Thanks for the feedback!
I used `Object` because `CloseableVerificationKeyResolver` extends jose4j's
`VerificationKeyResolver`, so importing it would trigger jose4j class loading.
However, looking at this more carefully, I realized this constructor is only
in tests and not in production code. Would it be better to remove this
constructor entirely and have the tests use `BrokerJwtValidator` directly?
This is my first contribution to Kafka, so I may have overlooked simpler
solutions. I appreciate your guidance!
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/DefaultJwtValidator.java:
##########
@@ -33,33 +32,43 @@
/**
* This {@link JwtValidator} uses the delegation approach, instantiating and
delegating calls to a
- * more concrete implementation. The underlying implementation is determined
by the presence/absence
- * of the {@link VerificationKeyResolver}: if it's present, a {@link
BrokerJwtValidator} is
- * created, otherwise a {@link ClientJwtValidator} is created.
+ * more concrete implementation. The underlying implementation is determined
by the configuration:
+ * if a JWKS endpoint URL is configured or a verification key resolver is
provided,
+ * a {@link BrokerJwtValidator} is created, otherwise a {@link
ClientJwtValidator} is created.
+ *
+ * <p>Note: {@link BrokerJwtValidator} and its jose4j dependency are loaded
lazily via reflection
+ * to avoid {@link ClassNotFoundException} in client-only environments where
jose4j is not
+ * on the classpath.
*/
public class DefaultJwtValidator implements JwtValidator {
- private final Optional<CloseableVerificationKeyResolver>
verificationKeyResolver;
+ private static final String BROKER_JWT_VALIDATOR_CLASS =
+ "org.apache.kafka.common.security.oauthbearer.BrokerJwtValidator";
+
+ private static final String CLOSEABLE_VERIFICATION_KEY_RESOLVER_CLASS =
+
"org.apache.kafka.common.security.oauthbearer.internals.secured.CloseableVerificationKeyResolver";
+
+ private final Optional<Object> verificationKeyResolver;
private JwtValidator delegate;
public DefaultJwtValidator() {
this.verificationKeyResolver = Optional.empty();
}
- public DefaultJwtValidator(CloseableVerificationKeyResolver
verificationKeyResolver) {
+ public DefaultJwtValidator(Object verificationKeyResolver) {
Review Comment:
@kirktrue Thanks for the feedback!
I used `Object` because `CloseableVerificationKeyResolver` extends jose4j's
`VerificationKeyResolver`, so importing it would trigger jose4j class loading.
However, looking at this more carefully, I realized this constructor is only
in tests and not in production code. Would it be better to remove this
constructor entirely and have the tests use `BrokerJwtValidator` directly?
This is my first contribution to Kafka, so I may have overlooked simpler
solutions. I appreciate your guidance!
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
