feat(azure): support custom OAuth scope and fix NPE for client_credentials tokens#1771
Open
954alberto wants to merge 6 commits into
Open
feat(azure): support custom OAuth scope and fix NPE for client_credentials tokens#1771954alberto wants to merge 6 commits into
954alberto wants to merge 6 commits into
Conversation
kapybro
Bot
added
status/triage
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughHandler now reads an optional JAAS Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as AzureEntraLoginCallbackHandler
participant JAAS as JAAS Entries
participant Config as Kafka Config
participant TokenCred as TokenCredential
participant Azure as Azure STS
Client->>Handler: configure(configs, mechanism, jaasEntries)
Handler->>JAAS: read JAAS options (scope, extension_*)
alt JAAS scope present
Handler->>TokenCred: build TokenRequestContext with JAAS scope
else
Handler->>Config: require exactly one bootstrap.servers (validated non-blank)
Handler->>TokenCred: build TokenRequestContext with https://<bootstrap-host>/.default
end
Handler->>JAAS: build SaslExtensions (if any)
Client->>Handler: handle(callbacks)
alt includes SaslExtensionsCallback
Handler-->>Client: provide saslExtensions
end
alt includes OAuthBearerTokenCallback
Handler->>TokenCred: getToken(TokenRequestContext)
TokenCred->>Azure: request token
Azure-->>TokenCred: token (or empty)
TokenCred-->>Handler: token (or null)
alt token present
Handler-->>Client: oauthCallback.token(token)
else
Handler-->>Client: oauthCallback.error("invalid_grant","Token acquisition returned empty result.",null)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi 954alberto! 👋
Welcome, and thank you for opening your first PR in the repo!
Please wait for triaging by our maintainers.
Please take a look at our contributing guide.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
api/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraOAuthBearerTokenTest.java (1)
40-55: Consider simplifying JSON-like payload comments to avoid Sonar “commented-out code” warnings.The inline pseudo-JSON comments are informative but can be flagged as dead commented code by static analysis.
Possible cleanup
- // Payload: {"aud":"https://sample.servicebus.windows.net","iss":"https://sts.windows.net/sample/", - // "iat":1698415912,"nbf":1698415913,"exp":1698415914,"appid":"sample-app-id", - // "sub":"Sample Subject","tid":"sample-tid"} + // Includes aud/iss/iat/nbf/exp/appid/sub/tid claims (no scp, no upn). @@ - // Payload: {"aud":"https://sample.servicebus.windows.net","iss":"https://sts.windows.net/sample/", - // "iat":1698415912,"nbf":1698415913,"exp":1698415914,"sub":"Sample Subject"} + // Includes aud/iss/iat/nbf/exp/sub only (no scp, upn, appid).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraOAuthBearerTokenTest.java` around lines 40 - 55, The pseudo-JSON payload comments next to CLIENT_CREDENTIALS_TOKEN and MINIMAL_TOKEN in AzureEntraOAuthBearerTokenTest are being flagged as commented-out code; replace those multi-line JSON-like comments with a short descriptive sentence per token (e.g., "Client credentials token with appid and sub claims" and "Minimal token with standard iat/nbf/exp/sub claims") or collapse them into a single-line comment without JSON punctuation so Sonar won't treat them as dead code, keeping the current token variable names (CLIENT_CREDENTIALS_TOKEN, MINIMAL_TOKEN) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@api/src/main/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandler.java`:
- Around line 51-77: The JAAS scope handling accepts whitespace-only or empty
strings and stops at the first empty entry; modify getJaasOption to normalize
and skip blank values by trimming the String and only returning a scope when
trimmed length > 0, otherwise continue to the next AppConfigurationEntry (return
null if none found), so the customScope check in the method that builds
TokenRequestContext uses a genuinely non-blank scope and falls back to
buildTokenAudience when appropriate.
In
`@api/src/main/java/io/kafbat/ui/config/auth/azure/AzureEntraOAuthBearerToken.java`:
- Around line 62-70: The fallback cast (String) claims.getClaim("sub") is unsafe
and may throw ClassCastException; in AzureEntraOAuthBearerToken (the principal
resolution block using claims.getClaim("upn"/"appid"/"sub")), replace the raw
cast with an instanceof check for a String and if it's missing or not a String,
throw a controlled authentication error (e.g., an AuthenticationException or a
project-specific auth exception) with a clear message instead of letting a
ClassCastException bubble up.
In
`@api/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandlerTest.java`:
- Around line 221-246: The test should not pass "bootstrap.servers" because its
intent is to verify that when a custom scope is provided bootstrap validation is
skipped; change the configs used in
shouldNotRequireBootstrapServersWhenCustomScopeProvided() from
Map.of("bootstrap.servers", ...) to an empty Map (or omit the key) so
azureEntraLoginCallbackHandler.configure(configs, null, List.of(jaasEntry)) is
called with no bootstrap.servers and then run the same assertions (capture
TokenRequestContext from tokenCredential.getToken and assert scopes match
customScope).
---
Nitpick comments:
In
`@api/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraOAuthBearerTokenTest.java`:
- Around line 40-55: The pseudo-JSON payload comments next to
CLIENT_CREDENTIALS_TOKEN and MINIMAL_TOKEN in AzureEntraOAuthBearerTokenTest are
being flagged as commented-out code; replace those multi-line JSON-like comments
with a short descriptive sentence per token (e.g., "Client credentials token
with appid and sub claims" and "Minimal token with standard iat/nbf/exp/sub
claims") or collapse them into a single-line comment without JSON punctuation so
Sonar won't treat them as dead code, keeping the current token variable names
(CLIENT_CREDENTIALS_TOKEN, MINIMAL_TOKEN) intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b529fb10-dfb4-40c8-911c-34a6916b13c2
📒 Files selected for processing (4)
api/src/main/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandler.javaapi/src/main/java/io/kafbat/ui/config/auth/azure/AzureEntraOAuthBearerToken.javaapi/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandlerTest.javaapi/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraOAuthBearerTokenTest.java
954alberto
force-pushed
the
feat/azure-entra-oauthbearer-improvements
branch
from
58b20e2 to
39c717f
Compare
13 tasks
Haarolean
previously approved these changes
Apr 6, 2026
Haarolean
added
type/enhancement
added 3 commits
April 10, 2026 10:38
…tials tokens The AzureEntraLoginCallbackHandler previously derived the token scope exclusively from the bootstrap server URL, which only works for Azure Event Hubs. Services like Confluent Cloud require a different scope (e.g. api://<client-id>/.default). This commit adds a 'scope' JAAS config option that, when set, overrides the auto-derived scope. When not set, behavior is unchanged (backward compatible). Additionally, AzureEntraOAuthBearerToken.scope() and principalName() threw NullPointerException for client_credentials (app-only) tokens because these tokens lack the 'scp' and 'upn' claims that are only present in delegated (user) tokens. The fix gracefully handles missing claims by returning an empty scope set and falling back to 'appid' or 'sub' for the principal name. Signed-off-by: Alberto <954alberto@users.noreply.github.com>
- Trim and skip blank JAAS scope values in getJaasOption - Use instanceof + isBlank checks for all principal claim fallbacks - Throw SaslAuthenticationException when no principal claim is found - Fix test to use empty configs when verifying custom scope bypasses bootstrap.servers validation - Simplify token constant comments to avoid static analysis warnings - Add test for SaslAuthenticationException on missing principal claims
- Add @nullable to getJaasOption() return type (reviewer request) - Fix bootstrapServers.size() > 1 to != 1: an empty list would bypass validation and throw IndexOutOfBoundsException at .get(0) - Guard .block() null result in handleOAuthCallback: Reactor's block() is @nullable by contract; if the Mono completes empty, pass a controlled error to the callback instead of passing null to token() - Add tests for empty bootstrap servers list and empty token Mono
954alberto
force-pushed
the
feat/azure-entra-oauthbearer-improvements
branch
from
39c717f to
e98d49d
Compare
Author
Changes in latest commit (e98d49d)Addressed reviewer feedback and fixed two additional bugs found during review: Reviewer feedback
Bug fixes
New tests
All existing and new tests pass (BUILD SUCCESSFUL). |
Author
|
@Haarolean All review comments have been addressed and resolved, CI is green — this is ready to merge whenever you get a chance. Thanks for the review! |
added 2 commits
April 12, 2026 09:59
…EARER The Confluent-patched Kafka client (7.9.x-ccs) sends a SaslExtensionsCallback during the SASL/OAUTHBEARER handshake to convey logicalCluster and identityPoolId extensions. Without handling this callback, the broker rejects auth with CLUSTER_ID_MISSING_OR_EMPTY. - Read extension_* JAAS options during configure() and store as SaslExtensions - Handle SaslExtensionsCallback in handle() alongside OAuthBearerTokenCallback - Add 4 new unit tests for extension parsing and callback dispatch
…cribeMetadataQuorum On Confluent Cloud with limited RBAC (no cluster-level Describe permission), describeFeatures() throws ClusterAuthorizationException which propagates unhandled through ConfigRelatedInfo.extract(), causing admin client creation to fail on every statistics poll cycle. - Wrap describeFeatures() in ConfigRelatedInfo.extract() with onErrorResume for ClusterAuthorizationException and UnsupportedVersionException, falling back to version detection via inter.broker.protocol.version only - Add ClusterAuthorizationException handling to loadQuorumInfo() alongside the existing UnsupportedVersionException handling
Author
|
We built a Docker image from this branch and tested it against Confluent Cloud using Azure Entra Workload Identity. Authentication, statistics collection, and the UI all work as expected. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/main/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandler.java (1)
128-150:⚠️ Potential issue | 🟡 MinorReject blank bootstrap server entries too.
Size validation alone still accepts
List.of("")or whitespace-only entries. That path can build a URI without a host, so the derived scope becomes malformed instead of failing fast with a clear config error.Proposed fix
if (bootstrapServers.size() != 1) { final String message = BOOTSTRAP_SERVERS_CONFIG + " must contain exactly one bootstrap server, but found " + bootstrapServers.size() + "."; log.error(message); throw new IllegalArgumentException(message); } - return URI.create("https://" + bootstrapServers.get(0)); + String bootstrapServer = bootstrapServers.get(0).trim(); + if (bootstrapServer.isEmpty()) { + final String message = + BOOTSTRAP_SERVERS_CONFIG + " must contain a non-blank bootstrap server."; + log.error(message); + throw new IllegalArgumentException(message); + } + + URI uri = URI.create("https://" + bootstrapServer); + if (uri.getHost() == null) { + final String message = + BOOTSTRAP_SERVERS_CONFIG + " must contain a valid host[:port] value, but found " + + bootstrapServer + "."; + log.error(message); + throw new IllegalArgumentException(message); + } + return uri;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/main/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandler.java` around lines 128 - 150, buildBootstrapServerUri currently only checks list size but allows blank or whitespace-only entries (e.g., List.of("")) which yields malformed URIs; update buildBootstrapServerUri to validate the single bootstrap server string is non-null, trimmed, and not empty/blank (reject entries where bootstrapServers.get(0).trim().isEmpty()), log a clear error referencing BOOTSTRAP_SERVERS_CONFIG and throw IllegalArgumentException if invalid, and only then create the URI (used by buildTokenAudience with TOKEN_AUDIENCE_FORMAT) so malformed scope construction is prevented.
🧹 Nitpick comments (1)
api/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandlerTest.java (1)
111-188: Add a regression case for blankscopevalues.These tests cover scope present/absent, but not the new trim-and-skip-blank behavior in
getJaasOption(). A whitespace-onlyscopeshould still fall back to the bootstrap-derived audience, and a blank earlier entry should not mask a later valid scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandlerTest.java` around lines 111 - 188, Add two regression tests in AzureEntraLoginCallbackHandlerTest: one that supplies a JAAS AppConfigurationEntry with "scope" set to whitespace-only (e.g. " ") and asserts that the TokenRequestContext scopes fall back to the bootstrap-derived audience (as in shouldFallBackToBootstrapDerivedScopeWithEmptyJaasList), and another that supplies multiple JAAS entries where an earlier entry has an empty/whitespace "scope" and a later entry has a valid custom scope and assert the valid later scope is used; use the same pattern as existing tests (configure(configs,..., List.of(jaasEntry/...)), mock tokenCredential.getToken(...), capture TokenRequestContext via ArgumentCaptor, and verify azureEntraLoginCallbackHandler.handle uses the expected scopes) and reference azureEntraLoginCallbackHandler, oauthBearerTokenCallBack, tokenCredential and TokenRequestContext in the new tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/main/java/io/kafbat/ui/service/StatisticsService.java`:
- Around line 76-79: The current onErrorResume swallows
ClusterAuthorizationException and maps it to Optional.empty(), which later gets
interpreted as ZOOKEEPER; remove ClusterAuthorizationException from that branch
so authorization failures are not treated as "no quorum info." Update the
onErrorResume handling in StatisticsService.onErrorResume (the lambda
referencing UnsupportedVersionException, ClusterAuthorizationException, and
Optional.empty()) to only map UnsupportedVersionException to Optional.empty(),
and let ClusterAuthorizationException propagate (or return a distinct
marker/value the stat builder can interpret as "authorization
failure"/"unavailable") so the controller type rendering can distinguish auth
failures from a non-quorum cluster.
---
Outside diff comments:
In
`@api/src/main/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandler.java`:
- Around line 128-150: buildBootstrapServerUri currently only checks list size
but allows blank or whitespace-only entries (e.g., List.of("")) which yields
malformed URIs; update buildBootstrapServerUri to validate the single bootstrap
server string is non-null, trimmed, and not empty/blank (reject entries where
bootstrapServers.get(0).trim().isEmpty()), log a clear error referencing
BOOTSTRAP_SERVERS_CONFIG and throw IllegalArgumentException if invalid, and only
then create the URI (used by buildTokenAudience with TOKEN_AUDIENCE_FORMAT) so
malformed scope construction is prevented.
---
Nitpick comments:
In
`@api/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandlerTest.java`:
- Around line 111-188: Add two regression tests in
AzureEntraLoginCallbackHandlerTest: one that supplies a JAAS
AppConfigurationEntry with "scope" set to whitespace-only (e.g. " ") and
asserts that the TokenRequestContext scopes fall back to the bootstrap-derived
audience (as in shouldFallBackToBootstrapDerivedScopeWithEmptyJaasList), and
another that supplies multiple JAAS entries where an earlier entry has an
empty/whitespace "scope" and a later entry has a valid custom scope and assert
the valid later scope is used; use the same pattern as existing tests
(configure(configs,..., List.of(jaasEntry/...)), mock
tokenCredential.getToken(...), capture TokenRequestContext via ArgumentCaptor,
and verify azureEntraLoginCallbackHandler.handle uses the expected scopes) and
reference azureEntraLoginCallbackHandler, oauthBearerTokenCallBack,
tokenCredential and TokenRequestContext in the new tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 39e8e725-4fc8-40e7-8e4f-0b37de775358
📒 Files selected for processing (4)
api/src/main/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandler.javaapi/src/main/java/io/kafbat/ui/service/ReactiveAdminClient.javaapi/src/main/java/io/kafbat/ui/service/StatisticsService.javaapi/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandlerTest.java
- Validate blank bootstrap servers (trim + empty check) - Separate ClusterAuthorizationException handling in loadQuorumInfo with WARN log - Add tests for blank bootstrap server and whitespace-only scope
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandlerTest.java (1)
111-395: Consider extracting repeated test fixtures into helpers/constants.There’s repeated setup for bootstrap config maps, JAAS entry creation, and default scope literals. Pulling these into helper methods/constants would reduce maintenance overhead and keep each test focused on intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandlerTest.java` around lines 111 - 395, Tests in AzureEntraLoginCallbackHandlerTest repeat bootstrap/config and JAAS entry setup; extract common fixtures into helpers and constants to reduce duplication: add private helper methods (e.g., createBootstrapConfig(String hostPort), createJaasEntry(String scope, Map<String,String> extensions)) and constants for common values (e.g., DEFAULT_EVENTHUB_SCOPE = "https://%s/.default", VALID_SAMPLE_TOKEN) and use them across tests instead of inline Map.of/ new AppConfigurationEntry and repeated scope literals; update tests like shouldUseCustomScopeFromJaasConfig, shouldFallBackToBootstrapDerivedScopeWhenJaasHasNoScope, shouldFallBackToBootstrapDerivedScopeWithEmptyJaasList, shouldFallBackToBootstrapDerivedScopeWhenScopeIsWhitespace, shouldNotRequireBootstrapServersWhenCustomScopeProvided, shouldHandleSaslExtensionsCallback, shouldReturnEmptyExtensionsWhenNoExtensionOptionsInJaas, shouldReturnEmptyExtensionsWhenNullJaasEntries, and shouldHandleBothTokenAndExtensionsCallbacksTogether to call these helpers/constants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@api/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandlerTest.java`:
- Around line 111-395: Tests in AzureEntraLoginCallbackHandlerTest repeat
bootstrap/config and JAAS entry setup; extract common fixtures into helpers and
constants to reduce duplication: add private helper methods (e.g.,
createBootstrapConfig(String hostPort), createJaasEntry(String scope,
Map<String,String> extensions)) and constants for common values (e.g.,
DEFAULT_EVENTHUB_SCOPE = "https://%s/.default", VALID_SAMPLE_TOKEN) and use them
across tests instead of inline Map.of/ new AppConfigurationEntry and repeated
scope literals; update tests like shouldUseCustomScopeFromJaasConfig,
shouldFallBackToBootstrapDerivedScopeWhenJaasHasNoScope,
shouldFallBackToBootstrapDerivedScopeWithEmptyJaasList,
shouldFallBackToBootstrapDerivedScopeWhenScopeIsWhitespace,
shouldNotRequireBootstrapServersWhenCustomScopeProvided,
shouldHandleSaslExtensionsCallback,
shouldReturnEmptyExtensionsWhenNoExtensionOptionsInJaas,
shouldReturnEmptyExtensionsWhenNullJaasEntries, and
shouldHandleBothTokenAndExtensionsCallbacksTogether to call these
helpers/constants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49388ff4-66a6-473b-b25d-0a6f7ffc1435
📒 Files selected for processing (3)
api/src/main/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandler.javaapi/src/main/java/io/kafbat/ui/service/StatisticsService.javaapi/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandlerTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- api/src/main/java/io/kafbat/ui/service/StatisticsService.java
- api/src/main/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandler.java
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandlerTest.java (1)
252-303: Add one regression test for multiple JAAS entries.The critical edge case in
getJaasOption(...)is “first entry has blankscope, later entry has a valid one.” The suite covers a single whitespace-only entry, but not the cross-entry fallback that this implementation now relies on. A small test here would lock that behavior in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandlerTest.java` around lines 252 - 303, Add a regression test in AzureEntraLoginCallbackHandlerTest that ensures multiple JAAS entries are handled so a blank/whitespace "scope" in the first AppConfigurationEntry falls back to a later entry's valid scope: create configs (can be empty), build a List.of two AppConfigurationEntry objects where the first has Map.of("scope"," ") and the second has Map.of("scope", validScope), stub tokenCredential.getToken(...) and accessToken.getToken(), call azureEntraLoginCallbackHandler.configure(..., List.of(...)) and handle(...), capture the TokenRequestContext via ArgumentCaptor and assert that getScopes() equals List.of(validScope); reference azureEntraLoginCallbackHandler, AppConfigurationEntry, tokenCredential and TokenRequestContext to locate relevant code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@api/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandlerTest.java`:
- Around line 252-303: Add a regression test in
AzureEntraLoginCallbackHandlerTest that ensures multiple JAAS entries are
handled so a blank/whitespace "scope" in the first AppConfigurationEntry falls
back to a later entry's valid scope: create configs (can be empty), build a
List.of two AppConfigurationEntry objects where the first has Map.of("scope","
") and the second has Map.of("scope", validScope), stub
tokenCredential.getToken(...) and accessToken.getToken(), call
azureEntraLoginCallbackHandler.configure(..., List.of(...)) and handle(...),
capture the TokenRequestContext via ArgumentCaptor and assert that getScopes()
equals List.of(validScope); reference azureEntraLoginCallbackHandler,
AppConfigurationEntry, tokenCredential and TokenRequestContext to locate
relevant code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e61db436-2c38-4b23-a528-2d30d859392c
📒 Files selected for processing (3)
api/src/main/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandler.javaapi/src/main/java/io/kafbat/ui/service/StatisticsService.javaapi/src/test/java/io/kafbat/ui/config/auth/azure/AzureEntraLoginCallbackHandlerTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- api/src/main/java/io/kafbat/ui/service/StatisticsService.java
Member
could you please refrain from force-pushing a branch that has been already reviewed in the future? Doing so makes all the commits unreviewed |
Author
|
Hi @Haarolean, just checking in. All feedback has been addressed and CI is green. Would you be able to re-review when you get a chance? Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR makes two improvements to the Azure Entra OAUTHBEARER integration:
1. Custom OAuth scope via JAAS config option
The existing
AzureEntraLoginCallbackHandlerderives the token scope from the bootstrap server URL (e.g.https://<host>/.default). This works for Azure Event Hubs, but not for Confluent Cloud (or other providers), where the required scope is an application registration URI likeapi://<client-id>/.default.This change adds support for a
scopeoption in the JAAS config:When the
scopeoption is present, it is used directly. When absent, the handler falls back to deriving the scope from the bootstrap URL — so this is fully backward compatible.2. NPE fix for client_credentials (app-only) tokens
AzureEntraOAuthBearerTokenassumes Azure AD delegated (user) token structure:scope()callsclaims.getClaim("scp").split(" ")— but client_credentials tokens use therolesclaim instead ofscp, causing a NullPointerExceptionprincipalName()returnsclaims.getClaim("upn")— but client_credentials tokens have noupnclaim, returningnullFixed behavior:
scope(): checks ifscpclaim exists and is non-empty; returnsSet.of()if absentprincipalName(): falls back throughupn→appid→sub, throwingSaslAuthenticationExceptionif none are foundMotivation
The current
AzureEntraLoginCallbackHandlerhardcodes scope derivation from the bootstrap server URL, which only works for Azure Event Hubs. Any Kafka provider that requires a different OAuth scope (e.g. Confluent Cloud, or custom deployments with dedicated app registrations) cannot use this handler without modification.Similarly,
AzureEntraOAuthBearerTokenonly handles delegated (user) tokens. When used withclient_credentialsflow (common in service-to-service scenarios and Workload Identity), it throws NPE on missingscpand returnsnullforprincipalName().These changes make the Azure Entra integration work with any OAuth-compatible Kafka provider and any Azure AD token type.
Related
Changes
AzureEntraLoginCallbackHandler.javaJAAS_OPTION_SCOPEconstant,getJaasOption()method with trim/blank handling, custom scope logic inbuildTokenRequestContext(), renamedbuildEventHubsServerUri→buildBootstrapServerUriAzureEntraOAuthBearerToken.javascope()handles missingscpclaim gracefully;principalName()falls back throughupn→appid→subwithSaslAuthenticationExceptionif none foundAzureEntraLoginCallbackHandlerTest.javaAzureEntraOAuthBearerTokenTest.javaTesting
Summary by CodeRabbit
New Features
Bug Fixes
Tests