mneethiraj commented on code in PR #1000:
URL: https://github.com/apache/ranger/pull/1000#discussion_r3369974730


##########
security-admin/src/main/java/org/apache/ranger/biz/SessionMgr.java:
##########
@@ -219,7 +221,24 @@ public void resetUserModulePermission(UserSessionBase 
userSession) {
         XXUser xUser = 
daoManager.getXXUser().findByUserName(userSession.getLoginId());
 
         if (xUser != null) {
-            List<String>                         permissionList       = 
daoManager.getXXModuleDef().findAccessibleModulesByUserId(userSession.getUserId(),
 xUser.getId());
+            List<String> permissionList;
+
+            if (userSession.isUserAdmin() || userSession.isKeyAdmin()) {
+                List<XXModuleDef> allModules =

Review Comment:
   Instead of retrieving list of `XXModuleDef` objects, retrieving only 
attribute `module` (which is referenced in line 233 below) will be more 
efficient. Consider adding method `XXModuleDefDao.getAllModuleNames()` to 
return only the attribute needed here.



##########
security-admin/src/main/java/org/apache/ranger/biz/SessionMgr.java:
##########
@@ -556,6 +575,47 @@ private void setUserRoles(UserSessionBase userSession) {
             userSession.setUserAdmin(false);
         }
 
+        applyConfigSuperUserSessionFlags(userSession);
+
+        if (userSession.isSuperUser()) {
+            strRoleList = RangerSuperUserConfig.mergeConfigSuperUserRoles(

Review Comment:
   For readability, please unnecessary line splits like this - long gone are 
the days code was printed in paper, which limited line length to 80 or 132. 
Please review other updates in this PR.



##########
security-admin/src/main/java/org/apache/ranger/biz/SessionMgr.java:
##########
@@ -556,6 +575,47 @@ private void setUserRoles(UserSessionBase userSession) {
             userSession.setUserAdmin(false);
         }
 
+        applyConfigSuperUserSessionFlags(userSession);
+
+        if (userSession.isSuperUser()) {
+            strRoleList = RangerSuperUserConfig.mergeConfigSuperUserRoles(
+                    strRoleList, true);
+        }
+
         userSession.setUserRoleList(strRoleList);
     }
+
+    /**
+     * Applies config super-user session flag ({@code superUser}) when login 
matches
+     * {@code ranger.admin.super.users} / super.groups.
+     */
+    private void applyConfigSuperUserSessionFlags(
+            final UserSessionBase userSession) {
+        if (userSession == null) {
+            return;
+        }
+
+        String loginId = userSession.getLoginId();
+
+        if (loginId == null) {
+            return;
+        }
+
+        if (!RangerSuperUserConfig.isEnabled()) {
+            userSession.setSuperUser(false);
+            return;
+        }
+
+        Set<String> userGroups = xUserMgr.getSyncedGroupsForUser(loginId);

Review Comment:
   Consider calling `getSyncedGroupsForUser()` only when necessary.
   
   ```
   final boolean isSuperUser;
   
   if (RangerSuperUserConfig.isSuperGroupsConfigured()) {
     isSuperUser = RangerSuperUserConfig.isSuperUser(loginId, 
xUserMgr.getSyncedGroupsForUser(loginId));
   } else {
     isSuperUser = RangerSuperUserConfig.isSuperUser(loginId);
   }
   
   if (isSuperUser) {
     userSession.setSuperUser(true);
   
     logger.info("Granted full admin privileges via config for user {}", 
loginId);
   } else {
     userSession.setSuperUser(false);
   }
   ```



##########
security-admin/src/main/java/org/apache/ranger/common/RangerSuperUserConfig.java:
##########
@@ -0,0 +1,182 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.ranger.common;
+
+import org.apache.commons.collections.CollectionUtils;
+import org.apache.commons.lang3.StringUtils;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Set;
+
+/**
+ * Configuration-based Ranger Admin super users and super groups.
+ * When {@code ranger.admin.super.users} or
+ * {@code ranger.admin.super.groups} are set, matching authenticated users
+ * receive full Ranger administrative privileges (system admin and key admin
+ * capabilities) without requiring corresponding roles in the Ranger database.
+ */
+public final class RangerSuperUserConfig {
+    private RangerSuperUserConfig() {
+    }
+
+    /**
+     * @return true when {@code ranger.admin.super.users} or
+     *         {@code ranger.admin.super.groups} has at least one non-blank
+     *         entry
+     */
+    public static boolean isEnabled() {
+        return hasNonEmptyEntry(

Review Comment:
   Consider initializing in the constructor and return the value from 
`isEnabled()`; this will help avoid unnecessary overheads in each call.
   
   ```
   public final class RangerSuperUserConfig {
     private final boolean isEnabled;
     private final boolean isSuperUsersConfigured;
     private final boolean isSuperGroupsConfigured;
   
     private RangerSuperUserConfig() {
       isSuperUsersConfigured  = 
hasNonEmptyEntry(PropertiesUtil.getPropertyStringList(RangerConstants.RANGER_ADMIN_SUPER_USERS));
       isSuperGroupsConfigured = 
hasNonEmptyEntry(PropertiesUtil.getPropertyStringList(RangerConstants.RANGER_ADMIN_SUPER_GROUPS));
       isEnabled               = isSuperUsersConfigured || 
isSuperGroupsConfigured;
     }
   
     ...
   }
   ```



-- 
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]

Reply via email to