[#5146] fix(core): Support to rename and delete metadata object in the authorization plugin #5321

jerqi · 2024-10-28T11:29:55Z

What changes were proposed in this pull request?

Support to rename and delete metadata object in the authorization plugin

Why are the changes needed?

Fix: #5146

Does this PR introduce any user-facing change?

No.

How was this patch tested?

add new IT and UT

jerqi · 2024-10-30T10:01:35Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

@@ -588,6 +798,67 @@ void testAllowUseSchemaPrivilege() throws InterruptedException {
    metalake.deleteRole(roleName);
  }

+  @Test
+  void testDenyPrivileges() throws InterruptedException {


… in the authorization plugin

xunliu · 2024-10-30T12:12:42Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

@@ -527,6 +536,208 @@ void testDeleteAndRecreateRole() throws InterruptedException {
    metalake.deleteRole(roleName);
  }

+  @Test
+  void testDeleteAndRecreateMetadataObject() throws InterruptedException {
+    // Create a role with CREATE_SCHEMA privilege


I think need to add sparkSession.sql(SQL_CREATE_SCHEMA); in the here to throw doesn't have permission.

xunliu · 2024-10-30T12:17:34Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

+
+    // Clean up
+    catalog.asSchemas().dropSchema(schemaName, true);
+    metalake.deleteRole(roleName);


I think need to add these tests in the here to throw doesn't have permission.

SQL_CREATE_SCHEMA

SQL_DROP_SCHEMA

xunliu · 2024-10-30T12:20:23Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

+    waitForUpdatingPolicies();
+
+    // Create a schema
+    sparkSession.sql(SQL_CREATE_SCHEMA);


I think need to add sparkSession.sql(SQL_CREATA_TABLE); in the here to throw doesn't have permission.

xunliu · 2024-10-30T12:20:48Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

+    // Create a schema
+    sparkSession.sql(SQL_CREATE_SCHEMA);
+
+    // Set owner


I think need to add sparkSession.sql(SQL_DROP_SCHEMA); in the here to throw doesn't have permission.

xunliu · 2024-10-30T12:22:31Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

+    // Create a schema and a table
+    sparkSession.sql(SQL_CREATE_SCHEMA);
+    sparkSession.sql(SQL_USE_SCHEMA);
+    sparkSession.sql(SQL_CREATE_TABLE);


I think need to add these tests in the here

SQL_INSERT_TABLE

I added another test case.

xunliu · 2024-10-30T13:47:38Z

core/src/main/java/org/apache/gravitino/hook/SchemaHookDispatcher.java

+    // Schema doesn't support to rename operation now. So we don't need to change
+    // authorization plugin privileges, too.


I think need call authorization plugin interface in the here.

Schema doesn't support to rename.

xunliu · 2024-10-30T13:53:22Z

core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java

  }

  @Override
  public boolean dropTable(NameIdentifier ident) {
-    return dispatcher.dropTable(ident);
+    boolean dropped = dispatcher.dropTable(ident);
+    AuthorizationUtils.removeAuthorizationPluginPrivileges(ident, Entity.EntityType.TABLE);


removeAuthorizationPluginPrivileges look like remove AuthorizationPlugin's Privileges, maybe can change to authorizationPluginRemovePrivileges or removePrivilegesInAuthorizationPlugin?

prefer option 1.

xunliu · 2024-10-31T07:56:40Z

core/src/main/java/org/apache/gravitino/hook/CatalogHookDispatcher.java

+    AuthorizationUtils.authorizationPluginRemovePrivileges(ident, Entity.EntityType.CATALOG);
    return dispatcher.dropCatalog(ident);


I think maybe we can call dropCatalog(ident, /*default value*/) in there, to avoid duplicate call return dispatcher.dropCatalog(ident); ?

xunliu · 2024-10-31T08:01:50Z

core/src/main/java/org/apache/gravitino/hook/TableHookDispatcher.java

  }

  @Override
  public boolean dropTable(NameIdentifier ident) {
-    return dispatcher.dropTable(ident);
+    boolean dropped = dispatcher.dropTable(ident);
+    AuthorizationUtils.authorizationPluginRemovePrivileges(ident, Entity.EntityType.TABLE);


Maybe we needs add judgement in here

if (dropped) { AuthorizationUtils.authorizationPluginRemovePrivileges(ident, Entity.EntityType.TABLE); }

Also other place have same problem.

If we succeed to delete the table but fail to delete the policy in the authorization plugin. We can repeat to execute the operation to delete the policy but dispatcher may return zero.

xunliu · 2024-10-31T08:16:06Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

+    waitForUpdatingPolicies();
+    Assertions.assertThrows(AccessControlException.class, () -> sparkSession.sql(SQL_INSERT_TABLE));
+    Assertions.assertThrows(
+        AccessControlException.class, () -> sparkSession.sql(SQL_SELECT_TABLE).collectAsList());


I think better to check all SQL operations.

SQL_CREATE_SCHEMA

SQL_USE_SCHEMA

SQL_CREATE_TABLE

SQL_INSERT_TABLE

SQL_SELECT_TABLE

SQL_UPDATE_TABLE

SQL_DELETE_TABLE

SQL_DROP_TABLE

xunliu · 2024-10-31T08:24:03Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

+    Assertions.assertThrows(AccessControlException.class, () -> sparkSession.sql(SQL_INSERT_TABLE));
+    Assertions.assertThrows(
+        AccessControlException.class, () -> sparkSession.sql(SQL_SELECT_TABLE).collectAsList());
+


I think better to check all SQL operations.

SQL_CREATE_SCHEMA

SQL_USE_SCHEMA

SQL_CREATE_TABLE

SQL_INSERT_TABLE

SQL_SELECT_TABLE

SQL_UPDATE_TABLE

SQL_DELETE_TABLE

SQL_DROP_TABLE

xunliu · 2024-10-31T08:26:55Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

+    // Delete the role and fail to create schema
+    metalake.deleteRole(roleName);
+    waitForUpdatingPolicies();
+    ;


Please remove this ;

xunliu

LGTM

…e authorization plugin (#5321) ### What changes were proposed in this pull request? Support to rename and delete metadata object in the authorization plugin ### Why are the changes needed? Fix: #5146 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? add new IT and UT

… in the authorization plugin (apache#5321) ### What changes were proposed in this pull request? Support to rename and delete metadata object in the authorization plugin ### Why are the changes needed? Fix: apache#5146 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? add new IT and UT

jerqi changed the title ~~Issue 5146~~ [#5146] fix(core): Support to rename and delete metadata object in the authorization plugin Oct 28, 2024

jerqi marked this pull request as draft October 28, 2024 11:31

jerqi force-pushed the ISSUE-5146 branch from e712257 to 08d412e Compare October 28, 2024 11:43

jerqi self-assigned this Oct 28, 2024

jerqi added the branch-0.7 Automatically cherry-pick commit to branch-0.7 label Oct 28, 2024

jerqi force-pushed the ISSUE-5146 branch 6 times, most recently from 699500c to 243f4a2 Compare October 29, 2024 08:42

jerqi closed this Oct 29, 2024

jerqi reopened this Oct 29, 2024

jerqi force-pushed the ISSUE-5146 branch from 243f4a2 to decfa2c Compare October 30, 2024 10:00

jerqi commented Oct 30, 2024

View reviewed changes

jerqi force-pushed the ISSUE-5146 branch 2 times, most recently from 85ae637 to 63355f9 Compare October 30, 2024 11:02

[apache#5146] fix(core): Support to rename and delete metadata object…

1cc7a19

… in the authorization plugin

jerqi force-pushed the ISSUE-5146 branch from 63355f9 to 1cc7a19 Compare October 30, 2024 12:05

jerqi marked this pull request as ready for review October 30, 2024 12:05

jerqi requested review from jerryshao, xunliu and yuqi1129 October 30, 2024 12:07

add more cases

5d01018

xunliu reviewed Oct 30, 2024

View reviewed changes

jerqi added 2 commits October 30, 2024 22:04

address comment

442bdb9

rename

d9b9ed0

xunliu reviewed Oct 31, 2024

View reviewed changes

address comments

e853537

fix style

7a412ed

xunliu approved these changes Oct 31, 2024

View reviewed changes

xunliu merged commit 2fd6ec1 into apache:main Oct 31, 2024
26 checks passed

jerqi deleted the ISSUE-5146 branch October 31, 2024 11:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#5146] fix(core): Support to rename and delete metadata object in the authorization plugin #5321

[#5146] fix(core): Support to rename and delete metadata object in the authorization plugin #5321

jerqi commented Oct 28, 2024 •

edited

Loading

jerqi Oct 30, 2024

xunliu Oct 30, 2024

jerqi Oct 30, 2024

xunliu Oct 30, 2024

jerqi Oct 30, 2024

xunliu Oct 30, 2024

jerqi Oct 30, 2024

xunliu Oct 30, 2024

jerqi Oct 30, 2024

xunliu Oct 30, 2024

jerqi Oct 30, 2024

xunliu Oct 30, 2024

jerqi Oct 30, 2024

xunliu Oct 30, 2024

jerqi Oct 30, 2024

xunliu Oct 31, 2024

jerqi Oct 31, 2024

xunliu Oct 31, 2024

jerqi Oct 31, 2024

xunliu Oct 31, 2024

jerqi Oct 31, 2024

xunliu Oct 31, 2024

jerqi Oct 31, 2024

xunliu Oct 31, 2024

jerqi Oct 31, 2024

xunliu left a comment

		// Schema doesn't support to rename operation now. So we don't need to change
		// authorization plugin privileges, too.

		AuthorizationUtils.authorizationPluginRemovePrivileges(ident, Entity.EntityType.CATALOG);
		return dispatcher.dropCatalog(ident);

[#5146] fix(core): Support to rename and delete metadata object in the authorization plugin #5321

[#5146] fix(core): Support to rename and delete metadata object in the authorization plugin #5321

Conversation

jerqi commented Oct 28, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xunliu left a comment

Choose a reason for hiding this comment

jerqi commented Oct 28, 2024 •

edited

Loading