Jungzhang commented on code in PR #63411:
URL: https://github.com/apache/doris/pull/63411#discussion_r3315047431
##########
fe/fe-core/src/test/java/org/apache/doris/mysql/authenticate/ldap/LdapManagerTest.java:
##########
@@ -74,4 +100,23 @@ public void testCheckUserPasswd() {
mockClient(true, false);
Assert.assertFalse(ldapManager.checkUserPasswd(USER2, "123"));
}
+
+ @Test
+ public void testGetUserInfoWithLdapDefaultRoles() {
+ LdapManager ldapManager = new LdapManager();
+ Deencapsulation.setField(ldapManager, "ldapClient", ldapClient);
+ LdapConfig.ldap_default_roles = new String[] {LDAP_DEFAULT_ROLE,
MISSING_LDAP_DEFAULT_ROLE};
+ Role ldapGroupRole = new Role(LDAP_GROUP_ROLE);
+ Role ldapDefaultRole = new Role(LDAP_DEFAULT_ROLE);
+ mockClient(true, true, new
ArrayList<>(Arrays.asList(LDAP_GROUP_ROLE)));
+ try (MockedStatic<Env> envMockedStatic =
Mockito.mockStatic(Env.class)) {
+ mockAuth(envMockedStatic, ldapGroupRole, ldapDefaultRole);
+
+ LdapUserInfo ldapUserInfo = ldapManager.getUserInfo(USER1);
+ Assert.assertNotNull(ldapUserInfo);
+ Assert.assertTrue(ldapUserInfo.getRoles().contains(ldapGroupRole));
Review Comment:
Done. I split the test coverage into separate cases for fallback-only
behavior and explicit additive behavior with
`ldap_always_apply_default_roles=true`.
##########
fe/fe-core/src/main/java/org/apache/doris/mysql/authenticate/ldap/LdapManager.java:
##########
@@ -256,12 +257,8 @@ private Set<Role> getLdapGroupsRoles(String userName)
throws DdlException {
// get user ldap group. the ldap group name should be the same as the
doris role name
List<String> ldapGroups = ldapClient.getGroups(userName);
Set<Role> roles = Sets.newHashSet();
- for (String group : ldapGroups) {
- String qualifiedRole = group;
- if (Env.getCurrentEnv().getAuth().doesRoleExist(qualifiedRole)) {
-
roles.add(Env.getCurrentEnv().getAuth().getRoleByName(qualifiedRole));
- }
- }
+ addExistingRoles(roles, ldapGroups, false);
+ addExistingRoles(roles, Arrays.asList(LdapConfig.ldap_default_roles),
true);
Review Comment:
@seawinde Thanks for pointing this out. I agree that fallback-only is the
safer default behavior, because applying default roles unconditionally may
broaden privileges for users who already have LDAP group-derived Doris roles.
I updated the implementation accordingly:
- `ldap_default_roles` are now applied only when no effective LDAP
group-derived Doris role exists.
- Added `ldap_always_apply_default_roles`, defaulting to `false`.
- When `ldap_always_apply_default_roles=true`, Doris keeps the additive
behavior and grants `ldap_default_roles` even if LDAP group-derived Doris roles
already exist. This preserves the use case where deployments need a mandatory
baseline role for all LDAP-authenticated users.
- Online updates of both `ldap_default_roles` and
`ldap_always_apply_default_roles` refresh the LDAP user cache.
I also updated the tests to cover separate cases:
- no effective LDAP group role -> default role is applied
- LDAP group role exists -> default role is not applied by default
- `ldap_always_apply_default_roles=true` -> default role is applied together
with LDAP group role
This keeps the default behavior conservative while still allowing the
original additive behavior as an explicit opt-in.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
