lianetm commented on code in PR #19622:
URL: https://github.com/apache/kafka/pull/19622#discussion_r2078400315
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/OAuthBearerLoginCallbackHandler.java:
##########
@@ -179,28 +180,34 @@ public class OAuthBearerLoginCallbackHandler implements
AuthenticateCallbackHand
private Map<String, Object> moduleOptions;
- private AccessTokenRetriever accessTokenRetriever;
+ private JwtRetriever jwtRetriever;
- private AccessTokenValidator accessTokenValidator;
+ private JwtValidator jwtValidator;
private boolean isInitialized = false;
@Override
public void configure(Map<String, ?> configs, String saslMechanism,
List<AppConfigurationEntry> jaasConfigEntries) {
moduleOptions = JaasOptionsUtils.getOptions(saslMechanism,
jaasConfigEntries);
- AccessTokenRetriever accessTokenRetriever =
AccessTokenRetrieverFactory.create(configs, saslMechanism, moduleOptions);
- AccessTokenValidator accessTokenValidator =
AccessTokenValidatorFactory.create(configs, saslMechanism);
- init(accessTokenRetriever, accessTokenValidator);
+ JwtRetriever jwtRetriever = new DefaultJwtRetriever(configs,
saslMechanism, moduleOptions);
+ JwtValidator jwtValidator = new DefaultJwtValidator(configs,
saslMechanism);
+ init(jwtRetriever, jwtValidator);
}
- public void init(AccessTokenRetriever accessTokenRetriever,
AccessTokenValidator accessTokenValidator) {
- this.accessTokenRetriever = accessTokenRetriever;
- this.accessTokenValidator = accessTokenValidator;
+ public void init(JwtRetriever jwtRetriever, JwtValidator jwtValidator) {
Review Comment:
uhm the `OAuthBearerLoginCallbackHandler` class is part of the public API,
so changing params in this `public` method would be a breaking change not
accounted for in the KIP, right?
It's a tricky situation because the class of the params is not public API
(`AccessTokenRetriever`).
This init is only used internally, and I expect is not intended to be called
directly ever (only indirectly from the public API `configure`). So I would say
that having this `init` public is what's wrong in the first place?
Anyways, fact is that it is public API at the moment
(https://kafka.apache.org/40/javadoc/org/apache/kafka/common/security/oauthbearer/OAuthBearerLoginCallbackHandler.html),
so we need to amend the KIP I would expect, to remove this init overload from
the public API (move to package-private). Since it's based on non-public param
types it's probably safe to assume no one should be really using it (but not
sure if I'm missing something here). Thoughts?
--
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]
