xunliu commented on code in PR #5113:
URL: https://github.com/apache/gravitino/pull/5113#discussion_r1805719604


##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -181,47 +236,122 @@ public Boolean onRoleUpdated(Role role, RoleChange... 
changes) throws RuntimeExc
   @Override
   public Boolean onOwnerSet(MetadataObject metadataObject, Owner preOwner, 
Owner newOwner)
       throws RuntimeException {
-    RangerHelper.check(newOwner != null, "The newOwner must be not null");
+    Preconditions.checkArgument(newOwner != null, "The newOwner must be not 
null");
 
     // Add the user or group to the Ranger
+    String preOwnerUserName = null,
+        preOwnerGroupName = null,
+        newOwnerUserName = null,
+        newOwnerGroupName = null;
     AuditInfo auditInfo =
         AuditInfo.builder()
             .withCreator(PrincipalUtils.getCurrentPrincipal().getName())
             .withCreateTime(Instant.now())
             .build();
+    if (preOwner != null) {
+      if (preOwner.type() == Owner.Type.USER) {
+        preOwnerUserName = newOwner.name();
+      } else {
+        preOwnerGroupName = newOwner.name();
+      }
+    }
     if (newOwner.type() == Owner.Type.USER) {
+      newOwnerUserName = newOwner.name();
       UserEntity userEntity =
           UserEntity.builder()
               .withId(1L)
-              .withName(newOwner.name())
+              .withName(newOwnerUserName)
               .withRoleNames(Collections.emptyList())
               .withRoleIds(Collections.emptyList())
               .withAuditInfo(auditInfo)
               .build();
       onUserAdded(userEntity);
     } else {
+      newOwnerGroupName = newOwner.name();
       GroupEntity groupEntity =
           GroupEntity.builder()
               .withId(1L)
-              .withName(newOwner.name())
+              .withName(newOwnerGroupName)
               .withRoleNames(Collections.emptyList())
               .withRoleIds(Collections.emptyList())
               .withAuditInfo(auditInfo)
               .build();
       onGroupAdded(groupEntity);
     }
 
-    RangerPolicy policy = rangerHelper.findManagedPolicy(metadataObject);
-    try {
-      if (policy == null) {
-        policy = rangerHelper.addOwnerToNewPolicy(metadataObject, newOwner);
-        rangerClient.createPolicy(policy);
-      } else {
-        rangerHelper.updatePolicyOwner(policy, preOwner, newOwner);
-        rangerClient.updatePolicy(policy.getId(), policy);
-      }
-    } catch (RangerServiceException e) {
-      throw new RuntimeException(e);
+    List<RangerSecurableObject> rangerSecurableObjects = 
translateOwner(metadataObject);
+    String ownerRoleName;
+    switch (metadataObject.type()) {
+      case METALAKE:
+      case CATALOG:
+        // The metalake and catalog use role to manage the owner
+        if (metadataObject.type() == MetadataObject.Type.METALAKE) {
+          ownerRoleName = RangerHelper.GRAVITINO_METALAKE_OWNER_ROLE;
+        } else {
+          ownerRoleName = RangerHelper.GRAVITINO_CATALOG_OWNER_ROLE;
+        }
+        rangerHelper.createRangerRoleIfNotExists(ownerRoleName, true);
+        try {
+          if (preOwnerUserName != null || preOwnerGroupName != null) {
+            GrantRevokeRoleRequest revokeRoleRequest =
+                rangerHelper.createGrantRevokeRoleRequest(
+                    ownerRoleName, preOwnerUserName, preOwnerGroupName);
+            rangerClient.revokeRole(rangerServiceName, revokeRoleRequest);
+          }
+          if (newOwnerUserName != null || newOwnerGroupName != null) {
+            GrantRevokeRoleRequest grantRoleRequest =
+                rangerHelper.createGrantRevokeRoleRequest(
+                    ownerRoleName, newOwnerUserName, newOwnerGroupName);
+            rangerClient.grantRole(rangerServiceName, grantRoleRequest);
+          }
+        } catch (RangerServiceException e) {
+          // Ignore exception, support idempotent operation
+          LOG.warn("Grant owner role: {} failed!", ownerRoleName, e);
+        }
+
+        rangerSecurableObjects.stream()
+            .forEach(
+                rangerSecurableObject -> {
+                  RangerPolicy policy = 
rangerHelper.findManagedPolicy(rangerSecurableObject);
+                  try {
+                    if (policy == null) {
+                      policy =
+                          rangerHelper.addOwnerRoleToNewPolicy(
+                              rangerSecurableObject, ownerRoleName);
+                      rangerClient.createPolicy(policy);
+                    } else {
+                      rangerHelper.updatePolicyOwnerRole(policy, 
ownerRoleName);
+                      rangerClient.updatePolicy(policy.getId(), policy);
+                    }
+                  } catch (RangerServiceException e) {
+                    throw new RuntimeException(e);
+                  }
+                });
+        break;
+      case SCHEMA:
+      case TABLE:

Review Comment:
   Yes, We already have this check in the 
   ```
   protected RangerRole createRangerRoleIfNotExists(String roleName, boolean 
isOwnerRole) 
   ```



##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java:
##########
@@ -181,47 +236,122 @@ public Boolean onRoleUpdated(Role role, RoleChange... 
changes) throws RuntimeExc
   @Override
   public Boolean onOwnerSet(MetadataObject metadataObject, Owner preOwner, 
Owner newOwner)
       throws RuntimeException {
-    RangerHelper.check(newOwner != null, "The newOwner must be not null");
+    Preconditions.checkArgument(newOwner != null, "The newOwner must be not 
null");
 
     // Add the user or group to the Ranger
+    String preOwnerUserName = null,
+        preOwnerGroupName = null,
+        newOwnerUserName = null,
+        newOwnerGroupName = null;
     AuditInfo auditInfo =
         AuditInfo.builder()
             .withCreator(PrincipalUtils.getCurrentPrincipal().getName())
             .withCreateTime(Instant.now())
             .build();
+    if (preOwner != null) {
+      if (preOwner.type() == Owner.Type.USER) {
+        preOwnerUserName = newOwner.name();
+      } else {
+        preOwnerGroupName = newOwner.name();
+      }
+    }
     if (newOwner.type() == Owner.Type.USER) {
+      newOwnerUserName = newOwner.name();
       UserEntity userEntity =
           UserEntity.builder()
               .withId(1L)
-              .withName(newOwner.name())
+              .withName(newOwnerUserName)
               .withRoleNames(Collections.emptyList())
               .withRoleIds(Collections.emptyList())
               .withAuditInfo(auditInfo)
               .build();
       onUserAdded(userEntity);
     } else {
+      newOwnerGroupName = newOwner.name();
       GroupEntity groupEntity =
           GroupEntity.builder()
               .withId(1L)
-              .withName(newOwner.name())
+              .withName(newOwnerGroupName)
               .withRoleNames(Collections.emptyList())
               .withRoleIds(Collections.emptyList())
               .withAuditInfo(auditInfo)
               .build();
       onGroupAdded(groupEntity);
     }
 
-    RangerPolicy policy = rangerHelper.findManagedPolicy(metadataObject);
-    try {
-      if (policy == null) {
-        policy = rangerHelper.addOwnerToNewPolicy(metadataObject, newOwner);
-        rangerClient.createPolicy(policy);
-      } else {
-        rangerHelper.updatePolicyOwner(policy, preOwner, newOwner);
-        rangerClient.updatePolicy(policy.getId(), policy);
-      }
-    } catch (RangerServiceException e) {
-      throw new RuntimeException(e);
+    List<RangerSecurableObject> rangerSecurableObjects = 
translateOwner(metadataObject);
+    String ownerRoleName;
+    switch (metadataObject.type()) {
+      case METALAKE:
+      case CATALOG:
+        // The metalake and catalog use role to manage the owner
+        if (metadataObject.type() == MetadataObject.Type.METALAKE) {
+          ownerRoleName = RangerHelper.GRAVITINO_METALAKE_OWNER_ROLE;

Review Comment:
   Yes, We already have this check in the 
   ```
   protected RangerRole createRangerRoleIfNotExists(String roleName, boolean 
isOwnerRole) 
   ```



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