[GitHub] [hive] vihangk1 commented on a change in pull request #1391: Spark client can create/alter/drop a view

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

vihangk1 commented on a change in pull request #1391:
URL: https://github.com/apache/hive/pull/1391#discussion_r482240148



##########
File path: ql/src/test/org/apache/hadoop/hive/ql/security/authorization/plugin/metastore/TestHiveMetaStoreAuthorizer.java
##########
@@ -138,10 +140,37 @@ public void testC_CreateView_anyUser() throws Exception {
               .setOwner(authorizedUser)
               .build(conf);
       hmsHandler.create_table(viewObj);
+      Map<String, String> params = viewObj.getParameters();
+      assertTrue(params.containsKey("Authorized"));
+      assertTrue("false".equalsIgnoreCase(params.get("Authorized")));
     } catch (Exception e) {
-      String err = e.getMessage();
-      String expected = "Operation type CREATE_VIEW not allowed for user:" + authorizedUser;
-      assertEquals(expected, err);
+      // no Exceptions for user same as normal user is now allowed CREATE_VIEW operation

Review comment:
       The catch all exception here is not good, since the test will pass in case there is a MetaException thrown on line 142. The test added will pass without code modifications in the HiveMetastoreAuthorizer as well and hence I feel is not really a good regression test.
   
   Also, looks like there are other tests in this class which do a catch all exception blocks which can give false positive (eg. testD_CreateView_SuperUser). Would be good to fix them up as well.

##########
File path: ql/src/java/org/apache/hadoop/hive/ql/security/authorization/command/CommandAuthorizerV2.java
##########
@@ -121,6 +142,32 @@ private static void addPermanentFunctionEntities(SessionState ss, List<ReadEntit
     return hivePrivobjs;
   }
 
+  /**
+   * A deferred authorization view is view created by non-super user like spark-user. This view contains a parameter "Authorized"
+   * set to false, so ranger will not authorize it during view creation. When a select statement is issued, then the ranger authorizes
+   * the under lying tables.
+   * @param Table t
+   * @return boolean value
+   */
+  private static boolean isDeferredAuthView(Table t){
+    String tableType = t.getTTable().getTableType();
+    String authorizedKeyword = "Authorized";

Review comment:
       What I meant was having a constant defined in HiveMetastoreAuthorizer.java like below.
   public static final String DEFERRED_AUTHORIZED_KEY = "Authorized";
   
   Once you do that you can use the key DEFERRED_AUTHORIZED_KEY everywhere whereever you are directly looking for "Authorized" key. The advantage of doing this way is that code is less error-prone and maintainable.
   1. Future code modifications do introduce uppercase or lower case issues. (e.g. using "authorized" v/s "Authorized")
   2. Its easy to look for all the places where deferred authorized key is being used in the IDE by looking for the usages of the constant. Currently, we will have to do a git grep "Authorized" which is inconvenient.

##########
File path: ql/src/test/org/apache/hadoop/hive/ql/security/authorization/plugin/metastore/TestHiveMetaStoreAuthorizer.java
##########
@@ -138,10 +140,37 @@ public void testC_CreateView_anyUser() throws Exception {
               .setOwner(authorizedUser)
               .build(conf);
       hmsHandler.create_table(viewObj);
+      Map<String, String> params = viewObj.getParameters();
+      assertTrue(params.containsKey("Authorized"));
+      assertTrue("false".equalsIgnoreCase(params.get("Authorized")));
     } catch (Exception e) {
-      String err = e.getMessage();
-      String expected = "Operation type CREATE_VIEW not allowed for user:" + authorizedUser;
-      assertEquals(expected, err);
+      // no Exceptions for user same as normal user is now allowed CREATE_VIEW operation
+    }
+  }
+
+  @Test
+  public void testC2_AlterView_anyUser() throws Exception{
+    UserGroupInformation.setLoginUser(UserGroupInformation.createRemoteUser(authorizedUser));
+    try {
+      Table viewObj = new TableBuilder()
+              .setTableName(viewName)
+              .setType(TableType.VIRTUAL_VIEW.name())
+              .addCol("name", ColumnType.STRING_TYPE_NAME)
+              .setOwner(authorizedUser)
+              .build(conf);
+      hmsHandler.create_table(viewObj);
+      viewObj = new TableBuilder()
+              .setTableName(viewName)
+              .setType(TableType.VIRTUAL_VIEW.name())
+              .addCol("dep", ColumnType.STRING_TYPE_NAME)
+              .setOwner(authorizedUser)
+              .build(conf);
+      hmsHandler.alter_table("default", viewName, viewObj);
+      Map<String, String> params = viewObj.getParameters();
+      assertTrue(params.containsKey("Authorized"));
+      assertTrue("false".equalsIgnoreCase(params.get("Authorized")));
+    } catch (Exception e) {

Review comment:
       same comment as above related to exception handling.




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

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