Issues/798#1807
Open
Yaksha-sn wants to merge 3 commits into
Open
Conversation
…t#798) - Added checkUserHasRoles() in OAuthSecurityConfig to deny login when RBAC is enabled and user has no matching roles - Added same check in LdapSecurityConfig.RbacUserDetailsMapper - Default role (issue kafbat#344) is respected - users are allowed through if a defaultRole is configured even with no explicit role match - Added unit tests for both OAuth and LDAP paths
kapybro
Bot
added
status/triage
📝 WalkthroughWalkthroughThese changes implement role-based access enforcement for LDAP and OAuth2/OIDC authentication flows. When RBAC is enabled, users without matching roles and no configured default role are denied access via Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LdapProvider as LDAP Provider
participant AuthConfig as LdapSecurityConfig
participant RbacMapper as RbacUserDetailsMapper
participant ACS as AccessControlService
participant App as Application
User->>LdapProvider: Authenticate (username, password)
LdapProvider->>AuthConfig: Create UserDetails from LDAP context
AuthConfig->>RbacMapper: mapUserFromContext(context, user)
RbacMapper->>ACS: isRbacEnabled()?
alt RBAC Enabled
RbacMapper->>ACS: getRoles()
RbacMapper->>RbacMapper: Check user authorities against configured roles
alt User has matching role
RbacMapper->>RbacMapper: Create RbacLdapUser (success)
RbacMapper->>AuthConfig: Return RbacLdapUser
else No matching role
RbacMapper->>ACS: getDefaultRole()
alt Default role exists
RbacMapper->>RbacMapper: Create RbacLdapUser (success)
RbacMapper->>AuthConfig: Return RbacLdapUser
else No default role
RbacMapper->>User: throw AccessDeniedException
User->>App: ✗ Access Denied
end
end
else RBAC Disabled
RbacMapper->>RbacMapper: Create RbacLdapUser (success)
RbacMapper->>AuthConfig: Return RbacLdapUser
end
AuthConfig->>App: ✓ Authentication complete
App->>User: Login successful
sequenceDiagram
participant User
participant OAuthProvider as OAuth/OIDC Provider
participant AuthConfig as OAuthSecurityConfig
participant RoleValidator as checkUserHasRoles
participant ACS as AccessControlService
participant App as Application
User->>OAuthProvider: Initiate OAuth/OIDC login
OAuthProvider->>AuthConfig: User service callback
AuthConfig->>AuthConfig: Extract provider authority (groups)
AuthConfig->>RoleValidator: checkUserHasRoles(extracted groups)
RoleValidator->>ACS: isRbacEnabled()?
alt RBAC Enabled
RoleValidator->>ACS: getRoles()
RoleValidator->>RoleValidator: Match user groups against configured roles
alt User group matches role
RoleValidator->>AuthConfig: ✓ Return (allow)
else No matching group
RoleValidator->>ACS: getDefaultRole()
alt Default role exists
RoleValidator->>AuthConfig: ✓ Return (allow)
else No default role
RoleValidator->>User: throw AccessDeniedException
User->>App: ✗ Access Denied
end
end
else RBAC Disabled
RoleValidator->>AuthConfig: ✓ Return (allow)
end
AuthConfig->>AuthConfig: Create RbacOAuth2User/RbacOidcUser
AuthConfig->>App: ✓ Authentication complete
App->>User: Login successful
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 Yaksha-sn! 👋
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: 2
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/OAuthSecurityConfig.java (1)
148-186:⚠️ Potential issue | 🟠 MajorReplace
AccessDeniedExceptionwithOAuth2AuthenticationExceptionfor proper reactive OAuth2 failure handling.Spring Security's
ReactiveOAuth2UserServicecontract explicitly requiresOAuth2AuthenticationExceptionfor user-loading errors, notAccessDeniedException. ThrowingAccessDeniedExceptionfrom your custom user service (lines 225–226) will be handled byServerAccessDeniedHandler(403 Forbidden response), bypassing the OAuth2 login failure handler. This skips the configured/login?errorflow and likely renders a generic 403 error page instead. Wrap the denial inOAuth2AuthenticationExceptionwith an appropriateOAuth2Errorto integrate cleanly with Spring Security's reactive OAuth2 failure handling.Additionally, add the username to the error message (as in
LdapSecurityConfig) for better forensics when operators investigate failed login attempts.Suggested fix
- throw new AccessDeniedException( - "Access denied: authenticated user has no roles assigned."); + OAuth2Error oauth2Error = new OAuth2Error( + "insufficient_scope", + "Access denied: authenticated user has no roles assigned.", + null); + throw new OAuth2AuthenticationException(oauth2Error, oauth2Error.getDescription());🤖 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/OAuthSecurityConfig.java` around lines 148 - 186, In OAuthSecurityConfig, customOidcUserService and customOauth2UserService currently surface an AccessDeniedException via checkUserHasRoles which causes a 403 instead of triggering the OAuth2 failure flow; change the logic so when a user lacks roles you throw an OAuth2AuthenticationException constructed with a meaningful OAuth2Error (e.g. error code "access_denied") and include the username in the exception message (similar to LdapSecurityConfig) so the reactive OAuth2 login failure handler can process it; update the code paths that call checkUserHasRoles (and/or modify checkUserHasRoles) to produce OAuth2AuthenticationException with OAuth2Error rather than AccessDeniedException when invoked from customOidcUserService/customOauth2UserService.
🧹 Nitpick comments (1)
api/src/main/java/io/kafbat/ui/config/auth/LdapSecurityConfig.java (1)
183-205: Consider consolidating the duplicated RBAC role-check intoAccessControlService.The
!hasRole && defaultRole == nullpredicate is now implemented in two places — here and inOAuthSecurityConfig.checkUserHasRoles(lines 218-228) — with slightly different error messages. This is a natural fit forAccessControlService(it already ownsgetRoles(),getDefaultRole(), and afilterRole(user)predicate per the context snippet). A single method likeassertUserHasRoleOrDefault(Collection<String> groups, String username)would:
- keep the OAuth and LDAP branches behaviorally identical (including logging, exception type, and message format),
- let
AccessControlServiceown the RBAC semantics end-to-end (easier to change policy later, e.g. adding admin-override roles), and- remove the need to widen
RbacUserDetailsMappervisibility /OAuthSecurityConfig.checkUserHasRolespackage visibility purely for testability — you can test the centralized method directly.🤖 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/LdapSecurityConfig.java` around lines 183 - 205, Extract the duplicated RBAC predicate into AccessControlService by adding a method like assertUserHasRoleOrDefault(Collection<String> groups, String username) (or similar name) that checks getRoles()/getDefaultRole() and throws the canonical AccessDeniedException with the unified message; then replace the role-check logic in RbacUserDetailsMapper.mapUserFromContext and in OAuthSecurityConfig.checkUserHasRoles to call that new AccessControlService method (use existing symbols acs, RbacUserDetailsMapper, and OAuthSecurityConfig.checkUserHasRoles) so both branches share identical behavior, logging and exception text.
🤖 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/OAuthSecurityConfig.java`:
- Around line 218-228: Update checkUserHasRoles to accept the authenticated
username (e.g., add a String username parameter), make the method static, and
before throwing the AccessDeniedException log a warning with the username and
the extracted groups using the class logger; also include the username in the
thrown message (e.g., "Access denied: user 'X' has no roles assigned. Groups:
[...]"). Keep the RBAC check logic using AccessControlService acs and groups
as-is (method name checkUserHasRoles, parameters groups and acs), and update all
call sites to pass the OAuth/Oidc/OAuth2 user’s username so tests can invoke the
static helper cleanly.
In
`@api/src/test/java/io/kafbat/ui/config/auth/LdapSecurityConfigRoleCheckTest.java`:
- Around line 36-53: The test currently duplicates the RBAC logic in
checkAccess() instead of invoking the production code; update the tests to call
RbacUserDetailsMapper.mapUserFromContext(...) (or a spy of
RbacUserDetailsMapper) so the real role-check path is exercised; replace usages
of checkAccess(...) with a call to
mapper.mapUserFromContext(mockDirContextOperations, "testuser") while
mocking/stubbing the DirContextOperations or spying the mapper to return a
controllable UserDetails from super.mapUserFromContext(), and assert that the
mapper throws the real AccessDeniedException (and message) or returns the
expected mapped user when roles/defaultRole conditions are met.
---
Outside diff comments:
In `@api/src/main/java/io/kafbat/ui/config/auth/OAuthSecurityConfig.java`:
- Around line 148-186: In OAuthSecurityConfig, customOidcUserService and
customOauth2UserService currently surface an AccessDeniedException via
checkUserHasRoles which causes a 403 instead of triggering the OAuth2 failure
flow; change the logic so when a user lacks roles you throw an
OAuth2AuthenticationException constructed with a meaningful OAuth2Error (e.g.
error code "access_denied") and include the username in the exception message
(similar to LdapSecurityConfig) so the reactive OAuth2 login failure handler can
process it; update the code paths that call checkUserHasRoles (and/or modify
checkUserHasRoles) to produce OAuth2AuthenticationException with OAuth2Error
rather than AccessDeniedException when invoked from
customOidcUserService/customOauth2UserService.
---
Nitpick comments:
In `@api/src/main/java/io/kafbat/ui/config/auth/LdapSecurityConfig.java`:
- Around line 183-205: Extract the duplicated RBAC predicate into
AccessControlService by adding a method like
assertUserHasRoleOrDefault(Collection<String> groups, String username) (or
similar name) that checks getRoles()/getDefaultRole() and throws the canonical
AccessDeniedException with the unified message; then replace the role-check
logic in RbacUserDetailsMapper.mapUserFromContext and in
OAuthSecurityConfig.checkUserHasRoles to call that new AccessControlService
method (use existing symbols acs, RbacUserDetailsMapper, and
OAuthSecurityConfig.checkUserHasRoles) so both branches share identical
behavior, logging and exception text.
🪄 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: f9a17fdb-6b5d-4176-9196-821c62e145ee
📒 Files selected for processing (4)
api/src/main/java/io/kafbat/ui/config/auth/LdapSecurityConfig.javaapi/src/main/java/io/kafbat/ui/config/auth/OAuthSecurityConfig.javaapi/src/test/java/io/kafbat/ui/config/auth/LdapSecurityConfigRoleCheckTest.javaapi/src/test/java/io/kafbat/ui/config/auth/OAuthSecurityConfigRoleCheckTest.java
Comment on lines
+218
to
+228
| void checkUserHasRoles(Collection<String> groups, AccessControlService acs) { | ||
| if (!acs.isRbacEnabled()) { | ||
| return; | ||
| } | ||
| boolean hasRole = acs.getRoles().stream() | ||
| .anyMatch(role -> groups.contains(role.getName())); | ||
| if (!hasRole && acs.getDefaultRole() == null) { | ||
| throw new AccessDeniedException( | ||
| "Access denied: authenticated user has no roles assigned."); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a log line and include username in the denial message for parity with LDAP.
Two small polish items on the new helper:
- No log is emitted before throwing. Operators debugging “my user cannot log in” will see only a generic AccessDeniedException stack trace. A
log.warn(...)identifying the user and extracted groups would be valuable for RBAC troubleshooting (the exact symptom described in issue#798). LdapSecurityConfig.RbacUserDetailsMapperthrows with the username embedded ("... user '" + username + "' ..."), but this branch does not — the OAuthuser(OidcUser/OAuth2User) is in scope at the call sites and could be threaded through for consistency.
Also: the method has no instance-state dependency and could be static, which would make the test invocation cleaner.
🤖 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/OAuthSecurityConfig.java` around
lines 218 - 228, Update checkUserHasRoles to accept the authenticated username
(e.g., add a String username parameter), make the method static, and before
throwing the AccessDeniedException log a warning with the username and the
extracted groups using the class logger; also include the username in the thrown
message (e.g., "Access denied: user 'X' has no roles assigned. Groups: [...]").
Keep the RBAC check logic using AccessControlService acs and groups as-is
(method name checkUserHasRoles, parameters groups and acs), and update all call
sites to pass the OAuth/Oidc/OAuth2 user’s username so tests can invoke the
static helper cleanly.
Comment on lines
+36
to
+53
| /** | ||
| * Directly tests the role-check logic by constructing an RbacLdapUser | ||
| * with the given authorities and checking if access is granted or denied. | ||
| * This avoids calling super.mapUserFromContext() which requires a real LDAP | ||
| * context. | ||
| */ | ||
| private void checkAccess(Collection<GrantedAuthority> authorities) { | ||
| UserDetails userDetails = new User("testuser", "", authorities); | ||
| RbacLdapUser rbacUser = new RbacLdapUser(userDetails); | ||
|
|
||
| if (acs.isRbacEnabled()) { | ||
| boolean hasRole = acs.getRoles().stream() | ||
| .anyMatch(role -> rbacUser.groups().contains(role.getName())); | ||
| if (!hasRole && acs.getDefaultRole() == null) { | ||
| throw new AccessDeniedException("Access denied"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the mapper class is reachable from the test package (same package → accessible).
rg -nP '\b(static\s+)?class\s+RbacUserDetailsMapper\b' api/src/main/javaRepository: kafbat/kafka-ui
Length of output: 198
🏁 Script executed:
#!/bin/bash
# Extract the mapUserFromContext method from RbacUserDetailsMapper (around lines 195-202)
sed -n '180,210p' api/src/main/java/io/kafbat/ui/config/auth/LdapSecurityConfig.javaRepository: kafbat/kafka-ui
Length of output: 993
🏁 Script executed:
#!/bin/bash
# Extract the checkAccess method from the test file (lines 36-53)
sed -n '36,53p' api/src/test/java/io/kafbat/ui/config/auth/LdapSecurityConfigRoleCheckTest.javaRepository: kafbat/kafka-ui
Length of output: 795
🏁 Script executed:
#!/bin/bash
# Check the overall structure of the test class to understand how checkAccess is used
wc -l api/src/test/java/io/kafbat/ui/config/auth/LdapSecurityConfigRoleCheckTest.java
head -n 60 api/src/test/java/io/kafbat/ui/config/auth/LdapSecurityConfigRoleCheckTest.javaRepository: kafbat/kafka-ui
Length of output: 2343
🏁 Script executed:
#!/bin/bash
# Get the complete test file to see all test methods and their assertions
cat api/src/test/java/io/kafbat/ui/config/auth/LdapSecurityConfigRoleCheckTest.javaRepository: kafbat/kafka-ui
Length of output: 3821
Test duplicates the mapper's role-check logic instead of exercising the real code — regressions in RbacUserDetailsMapper will not be caught.
The checkAccess() helper reimplements the exact branch from RbacUserDetailsMapper.mapUserFromContext() (lines 195–202):
if (acs.isRbacEnabled()) {
boolean hasRole = acs.getRoles().stream()
.anyMatch(role -> rbacUser.groups().contains(role.getName()));
if (!hasRole && acs.getDefaultRole() == null) {
throw new AccessDeniedException("Access denied");
}
}The assertions in all five test methods therefore verify only that the helper matches the mocks, not that the production mapper does. This creates a coverage gap:
- If the mapper's exception message is changed (actual:
"Access denied: authenticated user '" + username + "' has no roles assigned."), tests pass. - If
super.mapUserFromContext()is accidentally omitted, tests pass. - If the role-matching predicate is refactored, divergence is only caught if the test's reimplementation is also changed in sync.
Since RBAC role enforcement is the core purpose of this PR (issue #798), exercise the real RbacUserDetailsMapper.mapUserFromContext() instead. Mock the required DirContextOperations parameter or spy on the mapper to stub the super call.
Suggested refactor
private void checkAccess(Collection<GrantedAuthority> authorities) {
- UserDetails userDetails = new User("testuser", "", authorities);
- RbacLdapUser rbacUser = new RbacLdapUser(userDetails);
-
- if (acs.isRbacEnabled()) {
- boolean hasRole = acs.getRoles().stream()
- .anyMatch(role -> rbacUser.groups().contains(role.getName()));
- if (!hasRole && acs.getDefaultRole() == null) {
- throw new AccessDeniedException("Access denied");
- }
- }
+ DirContextOperations ctx = mock(DirContextOperations.class);
+ LdapSecurityConfig.RbacUserDetailsMapper mapper =
+ Mockito.spy(new LdapSecurityConfig.RbacUserDetailsMapper(acs));
+ Mockito.doReturn(new User("testuser", "", authorities))
+ .when((LdapUserDetailsMapper) mapper)
+ .mapUserFromContext(eq(ctx), eq("testuser"), anyCollection());
+ mapper.mapUserFromContext(ctx, "testuser", authorities);
}🤖 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/LdapSecurityConfigRoleCheckTest.java`
around lines 36 - 53, The test currently duplicates the RBAC logic in
checkAccess() instead of invoking the production code; update the tests to call
RbacUserDetailsMapper.mapUserFromContext(...) (or a spy of
RbacUserDetailsMapper) so the real role-check path is exercised; replace usages
of checkAccess(...) with a call to
mapper.mapUserFromContext(mockDirContextOperations, "testuser") while
mocking/stubbing the DirContextOperations or spying the mapper to return a
controllable UserDetails from super.mapUserFromContext(), and assert that the
mapper throws the real AccessDeniedException (and message) or returns the
expected mapped user when roles/defaultRole conditions are met.
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
1 participant
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.
Fixes #798
Problem
When RBAC is enabled, a user who authenticates successfully via OAuth2 or LDAP
but matches no configured roles can still enter the UI, seeing an empty interface
with no clusters visible.
Solution
checkUserHasRoles()helper inOAuthSecurityConfigcalled after roleextraction in both
customOidcUserServiceandcustomOauth2UserServiceRbacUserDetailsMapper.mapUserFromContext()inLdapSecurityConfigAccessDeniedExceptionif RBAC is enabled, user has no matching roles,and no
defaultRoleis configureddefaultRole is configured
Tests
OAuthSecurityConfigRoleCheckTest— 7 unit tests covering OAuth pathLdapSecurityConfigRoleCheckTest— 5 unit tests covering LDAP pathSummary by CodeRabbit
New Features
Tests