luozenglin commented on code in PR #21968:
URL: https://github.com/apache/doris/pull/21968#discussion_r1270795322


##########
fe/fe-core/src/main/java/org/apache/doris/ldap/LdapManager.java:
##########
@@ -200,24 +207,22 @@ private LdapUserInfo getUserInfoFromCache(String 
fullName) {
      * Step2: get roles by ldap groups;
      * Step3: merge the roles;

Review Comment:
   Comments need to be updated



##########
fe/fe-core/src/main/java/org/apache/doris/ldap/LdapManager.java:
##########
@@ -200,24 +207,22 @@ private LdapUserInfo getUserInfoFromCache(String 
fullName) {
      * Step2: get roles by ldap groups;
      * Step3: merge the roles;
      */
-    private Role getLdapGroupsPrivs(String userName, String clusterName) 
throws DdlException {
+    private Set<Role> getLdapGroupsRoles(String userName, String clusterName) 
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);
-        List<String> rolesNames = Lists.newArrayList();
+        Set<Role> roles = Sets.newHashSet();
         for (String group : ldapGroups) {
             String qualifiedRole = ClusterNamespace.getFullName(clusterName, 
group);
             if (Env.getCurrentEnv().getAuth().doesRoleExist(qualifiedRole)) {
-                rolesNames.add(qualifiedRole);
+                
roles.add(Env.getCurrentEnv().getAuth().getRoleByName(qualifiedRole));
             }
         }
-        LOG.debug("get user:{} ldap groups:{} and doris roles:{}", userName, 
ldapGroups, rolesNames);
+        LOG.debug("get user:{} ldap groups:{} and doris roles:{}", userName, 
ldapGroups, roles);
 
         Role ldapGroupsPrivs = new Role(LDAP_GROUPS_PRIVS_NAME);

Review Comment:
   LDAP_GROUPS_PRIVS_NAME renamed to `defaultPrivs` or `defaultRole` or 
something else would be better, it can no longer represent the meaning of 
ldapGroupsPrivs. `infomation_schema select_priv` is also the default privilege 
of doris user, you can consider creating such a role to `RoleManager`, so that 
ldap user can save only the role name like doris user.



-- 
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: commits-unsubscr...@doris.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org

Reply via email to