kirktrue commented on code in PR #21518:
URL: https://github.com/apache/kafka/pull/21518#discussion_r2875589861
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/DefaultJwtValidator.java:
##########
@@ -84,4 +93,30 @@ public void close() throws IOException {
JwtValidator delegate() {
return delegate;
}
+
+ private static JwtValidator createBrokerJwtValidator() {
+ try {
+ Class<?> clazz = Class.forName(BROKER_JWT_VALIDATOR_CLASS);
+ return (JwtValidator) clazz.getDeclaredConstructor().newInstance();
+ } catch (ClassNotFoundException e) {
+ throw new RuntimeException(
+ "BrokerJwtValidator requires the jose4j library. Please add
org.bitbucket.b_c:jose4j to your classpath.", e);
+ } catch (NoSuchMethodException | InstantiationException |
IllegalAccessException | InvocationTargetException e) {
+ throw new RuntimeException("Failed to create BrokerJwtValidator",
e);
Review Comment:
Could you use `BROKER_JWT_VALIDATOR_CLASS` instead of hard-coding
`BrokerJwtValidator`? If a user sees `Failed to create BrokerJwtValidator` in
an error message, it may not be intuitive to troubleshoot.
##########
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) {
this.verificationKeyResolver = Optional.of(verificationKeyResolver);
}
@Override
public void configure(Map<String, ?> configs, String saslMechanism,
List<AppConfigurationEntry> jaasConfigEntries) {
if (verificationKeyResolver.isPresent()) {
- delegate = new BrokerJwtValidator(verificationKeyResolver.get());
+ delegate = createBrokerJwtValidator(verificationKeyResolver.get());
} else {
ConfigurationUtils cu = new ConfigurationUtils(configs,
saslMechanism);
Review Comment:
This is very unfortunate 😞
##########
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.
Review Comment:
👍
##########
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:
This is very unfortunate 😞
Can you add a comment to explain the reason that it's an `Object`?
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/DefaultJwtValidator.java:
##########
@@ -84,4 +93,30 @@ public void close() throws IOException {
JwtValidator delegate() {
return delegate;
}
+
+ private static JwtValidator createBrokerJwtValidator() {
+ try {
+ Class<?> clazz = Class.forName(BROKER_JWT_VALIDATOR_CLASS);
+ return (JwtValidator) clazz.getDeclaredConstructor().newInstance();
+ } catch (ClassNotFoundException e) {
+ throw new RuntimeException(
+ "BrokerJwtValidator requires the jose4j library. Please add
org.bitbucket.b_c:jose4j to your classpath.", e);
+ } catch (NoSuchMethodException | InstantiationException |
IllegalAccessException | InvocationTargetException e) {
+ throw new RuntimeException("Failed to create BrokerJwtValidator",
e);
+ }
+ }
+
+ private static JwtValidator createBrokerJwtValidator(Object
verificationKeyResolver) {
+ try {
+ Class<?> clazz = Class.forName(BROKER_JWT_VALIDATOR_CLASS);
+ Class<?> resolverClass =
Class.forName(CLOSEABLE_VERIFICATION_KEY_RESOLVER_CLASS);
+ Constructor<?> ctor = clazz.getDeclaredConstructor(resolverClass);
+ return (JwtValidator) ctor.newInstance(verificationKeyResolver);
+ } catch (ClassNotFoundException e) {
+ throw new RuntimeException(
+ "BrokerJwtValidator requires the jose4j library. Please add
org.bitbucket.b_c:jose4j to your classpath.", e);
+ } catch (NoSuchMethodException | InstantiationException |
IllegalAccessException | InvocationTargetException e) {
+ throw new RuntimeException("Failed to create BrokerJwtValidator",
e);
Review Comment:
Same change here regarding the message. Thanks
--
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]
