REVOKE GRANT OPTION FOR should revoke only GRANT OPTION

SipeiZhou · Dec 3, 2021 · 3df2371 · 3df2371
1 parent bc9a800
commit 3df2371
Show file tree

Hide file tree

Showing 6 changed files with 18 additions and 11 deletions.
diff --git a/...rc/main/java/io/trino/plugin/hive/metastore/thrift/FailureAwareThriftMetastoreClient.java b/...rc/main/java/io/trino/plugin/hive/metastore/thrift/FailureAwareThriftMetastoreClient.java
@@ -301,10 +301,10 @@ public boolean grantPrivileges(PrivilegeBag privilegeBag)
     }
 
     @Override
-    public boolean revokePrivileges(PrivilegeBag privilegeBag)
+    public boolean revokePrivileges(PrivilegeBag privilegeBag, boolean grantOption)
             throws TException
     {
-        return runWithHandle(() -> delegate.revokePrivileges(privilegeBag));
+        return runWithHandle(() -> delegate.revokePrivileges(privilegeBag, grantOption));
     }
 
     @Override

diff --git a/...n/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java b/...n/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java
@@ -1518,7 +1518,7 @@ public void revokeTablePrivileges(String databaseName, String tableName, String
                                 return null;
                             }
 
-                            metastoreClient.revokePrivileges(buildPrivilegeBag(databaseName, tableName, grantee, privilegesToRevoke));
+                            metastoreClient.revokePrivileges(buildPrivilegeBag(databaseName, tableName, grantee, privilegesToRevoke), grantOption);
                         }
                         return null;
                     }));

diff --git a/...o-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java b/...o-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java
@@ -42,6 +42,7 @@
 import org.apache.hadoop.hive.metastore.api.GetRoleGrantsForPrincipalResponse;
 import org.apache.hadoop.hive.metastore.api.GetTableRequest;
 import org.apache.hadoop.hive.metastore.api.GetValidWriteIdsRequest;
+import org.apache.hadoop.hive.metastore.api.GrantRevokePrivilegeRequest;
 import org.apache.hadoop.hive.metastore.api.GrantRevokeRoleRequest;
 import org.apache.hadoop.hive.metastore.api.GrantRevokeRoleResponse;
 import org.apache.hadoop.hive.metastore.api.GrantRevokeType;
@@ -78,6 +79,7 @@
 import static io.trino.plugin.hive.metastore.MetastoreUtil.adjustRowCount;
 import static java.lang.String.format;
 import static java.util.Objects.requireNonNull;
+import static org.apache.hadoop.hive.metastore.api.GrantRevokeType.GRANT;
 import static org.apache.hadoop.hive.metastore.api.GrantRevokeType.REVOKE;
 import static org.apache.hadoop.hive.metastore.txn.TxnUtils.createValidTxnWriteIdList;
 
@@ -351,14 +353,16 @@ public void dropRole(String role)
     public boolean grantPrivileges(PrivilegeBag privilegeBag)
             throws TException
     {
-        return client.grant_privileges(privilegeBag);
+        return client.grant_revoke_privileges(new GrantRevokePrivilegeRequest(GRANT, privilegeBag)).isSuccess();
     }
 
     @Override
-    public boolean revokePrivileges(PrivilegeBag privilegeBag)
+    public boolean revokePrivileges(PrivilegeBag privilegeBag, boolean revokeGrantOption)
             throws TException
     {
-        return client.revoke_privileges(privilegeBag);
+        GrantRevokePrivilegeRequest grantRevokePrivilegeRequest = new GrantRevokePrivilegeRequest(REVOKE, privilegeBag);
+        grantRevokePrivilegeRequest.setRevokeGrantOption(revokeGrantOption);
+        return client.grant_revoke_privileges(grantRevokePrivilegeRequest).isSuccess();
     }
 
     @Override

diff --git a/...trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreClient.java b/...trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreClient.java
@@ -141,7 +141,7 @@ void dropRole(String role)
     boolean grantPrivileges(PrivilegeBag privilegeBag)
             throws TException;
 
-    boolean revokePrivileges(PrivilegeBag privilegeBag)
+    boolean revokePrivileges(PrivilegeBag privilegeBag, boolean revokeGrantOption)
             throws TException;
 
     void grantRole(String role, String granteeName, PrincipalType granteeType, String grantorName, PrincipalType grantorType, boolean grantOption)

diff --git a/...o-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/MockThriftMetastoreClient.java b/...o-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/MockThriftMetastoreClient.java
@@ -402,7 +402,7 @@ public boolean grantPrivileges(PrivilegeBag privilegeBag)
     }
 
     @Override
-    public boolean revokePrivileges(PrivilegeBag privilegeBag)
+    public boolean revokePrivileges(PrivilegeBag privilegeBag, boolean revokeGrantOption)
     {
         throw new UnsupportedOperationException();
     }

diff --git a/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestGrantRevoke.java b/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestGrantRevoke.java
@@ -159,11 +159,14 @@ public void testGrantRevokeWithGrantOption()
         aliceExecutor.executeQuery(format("REVOKE GRANT OPTION FOR SELECT ON %s FROM bob", tableName));
         assertQueryFailure(() -> bobExecutor.executeQuery(format("GRANT SELECT ON %s TO ROLE role1 ", tableName)))
                 .hasMessageContaining(format("Access Denied: Cannot grant privilege SELECT on table default.%s", tableName));
-        // TODO (https://github.com/trinodb/trino/issues/4455) Should revoke only GRANT OPTION
-        assertQueryFailure(() -> bobExecutor.executeQuery(format("SELECT * FROM %s", tableName)))
-                .hasMessageContaining(format("Access Denied: Cannot select from table default.%s", tableName));
+        assertThat(bobExecutor.executeQuery(format("SELECT * FROM %s", tableName))).hasNoRows();
         // Since Hive doesn't support REVOKE with CASCADE, charlie would still have access to table
         assertThat(charlieExecutor.executeQuery(format("SELECT * FROM %s", tableName))).hasNoRows();
+
+        // test GRANT WITH GRANT OPTION post revoke
+        assertQueryFailure(() -> aliceExecutor.executeQuery(format("GRANT SELECT ON %s TO bob WITH GRANT OPTION", tableName)))
+                // Updating a privilege with GRANT OPTION is not supported by Hive. https://issues.apache.org/jira/browse/HIVE-15689
+                .hasMessageContaining("Granting SELECT WITH GRANT OPTION is not supported while USER bob possesses SELECT");
     }
 
     @Test(groups = {AUTHORIZATION, PROFILE_SPECIFIC_TESTS})