KAFKA-18847: Refactor OAuth layer to improve reusability 1/N #19622

kirktrue · 2025-05-02T15:31:27Z

Rename AccessTokenRetriever and AccessTokenValidator to
JwtRetriever and JwtValidator, respectively. Also converting the
factory pattern classes AccessTokenRetrieverFactory and
AccessTokenValidatorFactory into delegate/wrapper classes
DefaultJwtRetriever and DefaultJwtValidator, respectively.

These are all internal changes, no configuration, user APIs, RPCs, etc.
were changed.

Rename AccessTokenRetriever and AccessTokenValidator to JwtRetriever and JwtValidator, respectively. Also converting the factory pattern classes AccessTokenRetrieverFactory and AccessTokenValidatorFactory into delegate/wrapper classes DefaultJwtRetriever and DefaultJwtValidator, respectively. These are all internal changes, no configuration, user APIs, RPCs, etc. were changed.

…by linter

lianetm

Thanks @kirktrue ! No major concerns, some comments for consideration

...c/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/JwtValidator.java

...n/java/org/apache/kafka/common/security/oauthbearer/OAuthBearerValidatorCallbackHandler.java

.../java/org/apache/kafka/common/security/oauthbearer/internals/secured/BrokerJwtValidator.java

.../java/org/apache/kafka/common/security/oauthbearer/internals/secured/ClientJwtValidator.java

...java/org/apache/kafka/common/security/oauthbearer/internals/secured/DefaultJwtValidator.java

...in/java/org/apache/kafka/common/security/oauthbearer/internals/secured/FileJwtRetriever.java

...in/java/org/apache/kafka/common/security/oauthbearer/internals/secured/HttpJwtRetriever.java

.../org/apache/kafka/common/security/oauthbearer/internals/secured/JwtValidatorFactoryTest.java

… close() methods from BrokerJwtValidator and ClientJwtValidator

…od instead of the clumsier validateBoolean() method

…ackHandlerTest

kirktrue · 2025-05-06T23:17:05Z

Thanks for the review, @lianetm! I've fixed the issues you've mentioned and tested locally, but I'll watch the result of the CI job. Thanks!

m1a2st

Thanks @kirktrue for this patch, a little comments

...ache/kafka/common/security/oauthbearer/internals/secured/VerificationKeyResolverFactory.java

...va/org/apache/kafka/common/security/oauthbearer/OAuthBearerValidatorCallbackHandlerTest.java

m1a2st · 2025-05-07T09:47:00Z

...java/org/apache/kafka/common/security/oauthbearer/internals/secured/DefaultJwtValidator.java

+            List<String> l = cu.get(SASL_OAUTHBEARER_EXPECTED_AUDIENCE);
+
+            if (l != null)
+                expectedAudiences = Set.copyOf(l);


Perhaps we could update the variable name l to something more descriptive for better readability.

😆 Yes, I'll update it. Thanks!

Updated. Thanks!

…arer/internals/secured/VerificationKeyResolverFactory.java Co-authored-by: Ken Huang <s7133700@gmail.com>

…arer/OAuthBearerValidatorCallbackHandlerTest.java Co-authored-by: Ken Huang <s7133700@gmail.com>

Fixed missing import from hastily taking GitHub PR suggestion

…wtValidator.init()

lianetm

Thanks for the updates @kirktrue ! One important bit I just noticed, lmk

lianetm · 2025-05-07T20:07:27Z

.../main/java/org/apache/kafka/common/security/oauthbearer/OAuthBearerLoginCallbackHandler.java

-    public void init(AccessTokenRetriever accessTokenRetriever, AccessTokenValidator accessTokenValidator) {
-        this.accessTokenRetriever = accessTokenRetriever;
-        this.accessTokenValidator = accessTokenValidator;
+    public void init(JwtRetriever jwtRetriever, JwtValidator jwtValidator) {


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?

So I would say that having this init public is what's wrong in the first place?

This method is only public by virtue of the awkwardness of the unit tests. I'd prefer to make the init() method package protected, or just remove it altogether. The existence of init() was not mentioned in the previous KIP (KIP-768), so my thought is that it can be changed without updating the KIP.

When we originally added these classes, the methods were package-protected for testing purposes:
Initial PR

Later, when the packages were reorganized, the methods were made public:
Follow-up PR

I agree that since these methods are only used in tests and invoked exclusively for testing, it’s reasonable to update or delete them.

…dler and OAuthBearerValidatorCallbackHandler

omkreddy · 2025-05-08T05:52:18Z

...java/org/apache/kafka/common/security/oauthbearer/internals/secured/DefaultJwtRetriever.java

@@ -36,32 +39,27 @@
 import static org.apache.kafka.common.security.oauthbearer.OAuthBearerLoginCallbackHandler.CLIENT_SECRET_CONFIG;
 import static org.apache.kafka.common.security.oauthbearer.OAuthBearerLoginCallbackHandler.SCOPE_CONFIG;

-public class AccessTokenRetrieverFactory  {
+public class DefaultJwtRetriever implements JwtRetriever {


Can we add Javadoc?

omkreddy · 2025-05-08T06:39:11Z

.../main/java/org/apache/kafka/common/security/oauthbearer/OAuthBearerLoginCallbackHandler.java

@@ -177,57 +178,41 @@ public class OAuthBearerLoginCallbackHandler implements AuthenticateCallbackHand

    private static final String EXTENSION_PREFIX = "extension_";

-    private Map<String, Object> moduleOptions;
+    protected Map<String, Object> moduleOptions;


Can we avoid protected in these public classes. maybe we can add package-private getters/etc.. for testing

github-actions bot added triage PRs from the community tools clients labels May 2, 2025

m1a2st added the ci-approved label May 2, 2025

kirktrue added 2 commits May 2, 2025 11:10

Updates from linter

088d49b

Update OAuthCompatibilityTool to fix import ordering problems caught …

aa47fe3

…by linter

lianetm reviewed May 6, 2025

View reviewed changes

kirktrue added 11 commits May 6, 2025 15:40

Changed wording in Initable's init() Javadoc

c45219b

Removed ambiguity in OAuthBearerValidatorCallbackHandler's log messages

3d1a2ba

Removed superfluous no-op init() overrides

5cb1de3

Fixed inconsistent Javadoc formatting in ClientJwtValidator

84e2129

Created a default, no-op close() implementation and removed the no-op…

35d3454

… close() methods from BrokerJwtValidator and ClientJwtValidator

Updated validateUrlencodeHeader to use ConfigurationUtils’ get() meth…

f298a16

…od instead of the clumsier validateBoolean() method

Added clarifying Javadoc for DefaultJwtValidator

e5e97c8

Fixed a missing phrase in the FileJwtRetriever documentation.

08ec47d

HttpJwtRetriever Javadoc cleanup

bddc0e2

Moved tests from JwtValidatorFactoryTest to OAuthBearerValidatorCallb…

4c48438

…ackHandlerTest

Fixed import linting issues

9882fbe

github-actions bot removed the triage PRs from the community label May 7, 2025

m1a2st reviewed May 7, 2025

View reviewed changes

kirktrue and others added 4 commits May 7, 2025 11:05

Update clients/src/main/java/org/apache/kafka/common/security/oauthbe…

395d779

…arer/internals/secured/VerificationKeyResolverFactory.java Co-authored-by: Ken Huang <s7133700@gmail.com>

Update clients/src/test/java/org/apache/kafka/common/security/oauthbe…

dbb4ac9

…arer/OAuthBearerValidatorCallbackHandlerTest.java Co-authored-by: Ken Huang <s7133700@gmail.com>

Update OAuthBearerValidatorCallbackHandlerTest.java

54e6f69

Fixed missing import from hastily taking GitHub PR suggestion

Clean up awkward code for expected audience configuration in DefaultJ…

e95660e

…wtValidator.init()

lianetm reviewed May 7, 2025

View reviewed changes

kirktrue added 2 commits May 7, 2025 18:09

Updates to remove the init() methods from OAuthBearerLoginCallbackHan…

df52393

…dler and OAuthBearerValidatorCallbackHandler

Moved createAccessKey to OAuthBearerTest for reuse

74fd8cd

omkreddy reviewed May 8, 2025

View reviewed changes

Refactoring for unit tests to make less public API surface area

4fa46c5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-18847: Refactor OAuth layer to improve reusability 1/N #19622

KAFKA-18847: Refactor OAuth layer to improve reusability 1/N #19622

kirktrue commented May 2, 2025 •

edited by github-actions bot

Loading

lianetm left a comment

kirktrue commented May 6, 2025

m1a2st left a comment

m1a2st May 7, 2025

kirktrue May 7, 2025

kirktrue May 7, 2025

lianetm left a comment

lianetm May 7, 2025

kirktrue May 7, 2025

omkreddy May 8, 2025

omkreddy May 8, 2025

omkreddy May 8, 2025

KAFKA-18847: Refactor OAuth layer to improve reusability 1/N #19622

Are you sure you want to change the base?

KAFKA-18847: Refactor OAuth layer to improve reusability 1/N #19622

Conversation

kirktrue commented May 2, 2025 • edited by github-actions bot Loading

lianetm left a comment

Choose a reason for hiding this comment

kirktrue commented May 6, 2025

m1a2st left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lianetm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirktrue commented May 2, 2025 •

edited by github-actions bot

Loading