[GitHub] [hive] maheshk114 commented on a diff in pull request #3264: HVIE-26199 Reduce FileSystem init during user impersonation

[GitBox <gi...@apache.org>]

maheshk114 commented on code in PR #3264:
URL: https://github.com/apache/hive/pull/3264#discussion_r867661240


##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -454,13 +456,24 @@ private static Set<SQLPrivTypeGrant> getPrivilegesFromFS(
     if (FileUtils.isOwnerOfFileHierarchy(fs, fileStatus, userName, recurse)) {
       privs.add(SQLPrivTypeGrant.OWNER_PRIV);
     }
-    if (FileUtils.isActionPermittedForFileHierarchy(fs, fileStatus, userName, FsAction.WRITE, recurse)) {
+    UserGroupInformation ugi = Utils.getUGI();
+    String currentUser = ugi.getShortUserName();
+    FileSystem fsAsUser = null;
+    UserGroupInformation proxyUser = null;
+    if (userName != null && !userName.equals(currentUser)) {
+      proxyUser = UserGroupInformation.createProxyUser(userName, UserGroupInformation.getLoginUser());
+      fsAsUser = FileUtils.getFsAsUser(fs, proxyUser);
+    }
+    if (FileUtils.isActionPermittedForFileHierarchy(fs, fileStatus, userName, FsAction.WRITE, recurse, fsAsUser)) {
       privs.add(SQLPrivTypeGrant.INSERT_NOGRANT);
       privs.add(SQLPrivTypeGrant.DELETE_NOGRANT);
     }
-    if (FileUtils.isActionPermittedForFileHierarchy(fs, fileStatus, userName, FsAction.READ, recurse)) {
+    if (FileUtils.isActionPermittedForFileHierarchy(fs, fileStatus, userName, FsAction.READ, recurse, fsAsUser)) {
       privs.add(SQLPrivTypeGrant.SELECT_NOGRANT);
     }
+    if (proxyUser != null) {
+      FileSystem.closeAllForUGI(proxyUser);

Review Comment:
   Should not it be done for failure cases also ?



##########
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java:
##########
@@ -5008,11 +5009,25 @@ static private boolean needToCopy(final HiveConf conf, Path srcf, Path destf, Fi
         boolean isOwned = FileUtils.isOwnerOfFileHierarchy(srcFs, srcs, configuredOwner, false);
         if (configuredOwner.equals(runningUser)) {
           // Check if owner has write permission, else it will have to copy
+          UserGroupInformation ugi = Utils.getUGI();
+          String currentUser = ugi.getShortUserName();
+          FileSystem fsAsUser = null;
+          UserGroupInformation proxyUser = null;
+          if (configuredOwner != null && !configuredOwner.equals(currentUser)) {
+            proxyUser = UserGroupInformation.createProxyUser(configuredOwner, UserGroupInformation.getLoginUser());

Review Comment:
   This block is used more than once. Can be moved to a common block. 



##########
ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java:
##########
@@ -5008,11 +5009,25 @@ static private boolean needToCopy(final HiveConf conf, Path srcf, Path destf, Fi
         boolean isOwned = FileUtils.isOwnerOfFileHierarchy(srcFs, srcs, configuredOwner, false);
         if (configuredOwner.equals(runningUser)) {
           // Check if owner has write permission, else it will have to copy
+          UserGroupInformation ugi = Utils.getUGI();
+          String currentUser = ugi.getShortUserName();
+          FileSystem fsAsUser = null;
+          UserGroupInformation proxyUser = null;
+          if (configuredOwner != null && !configuredOwner.equals(currentUser)) {
+            proxyUser = UserGroupInformation.createProxyUser(configuredOwner, UserGroupInformation.getLoginUser());
+            fsAsUser = FileUtils.getFsAsUser(srcFs, proxyUser);
+          }
           if (!(isOwned &&
               FileUtils.isActionPermittedForFileHierarchy(
-                  srcFs, srcs, configuredOwner, FsAction.WRITE, false))) {
+                  srcFs, srcs, configuredOwner, FsAction.WRITE, false, fsAsUser))) {
+            if (proxyUser != null) {
+              FileSystem.closeAllForUGI(proxyUser);

Review Comment:
   This can be done in a final block



##########
common/src/java/org/apache/hadoop/hive/common/FileUtils.java:
##########
@@ -408,10 +408,23 @@ public static FileStatus getPathOrParentThatExists(FileSystem fs, Path path) thr
     return getPathOrParentThatExists(fs, parentPath);
   }
 
-  public static void checkFileAccessWithImpersonation(final FileSystem fs, final FileStatus stat,
-      final FsAction action, final String user)
-      throws IOException, AccessControlException, InterruptedException, Exception {
-    checkFileAccessWithImpersonation(fs, stat, action, user, null);
+  public static void checkFileAccessWithImpersonation(final FileSystem fs, final FileStatus stat, final FsAction action,
+      final String user) throws IOException, AccessControlException, InterruptedException, Exception {
+    UserGroupInformation ugi = Utils.getUGI();
+    String currentUser = ugi.getShortUserName();
+    UserGroupInformation proxyUser = null;
+    FileSystem fsAsUser = null;
+    try {
+      if (user != null && !user.equals(currentUser)) {
+        proxyUser = UserGroupInformation.createProxyUser(user, UserGroupInformation.getLoginUser());
+        fsAsUser = FileUtils.getFsAsUser(fs, proxyUser);
+      }
+      checkFileAccessWithImpersonation(fs, stat, action, user, null, fsAsUser);

Review Comment:
   instead of fsAsUser, pass currentUser and do proxy user creation in checkFileAccessWithImpersonation method.



-- 
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: gitbox-unsubscribe@hive.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org