[#4886] feat(server,core): Supports to list roles by object #5023

jerqi · 2024-09-26T02:27:49Z

What changes were proposed in this pull request?

Supports to list roles by object

Why are the changes needed?

Fix: #4886

Does this PR introduce any user-facing change?

I will add the document later.

How was this patch tested?

Add new UT.

yuqi1129 · 2024-09-26T06:16:09Z

core/src/main/java/org/apache/gravitino/storage/relational/JDBCBackend.java

      case ROLE_GROUP_REL:
        if (identType == Entity.EntityType.ROLE) {
          return (List<E>) GroupMetaService.getInstance().listGroupsByRoleIdent(nameIdentifier);
        } else {
          throw new IllegalArgumentException(
-              String.format("ROLE_GROUP_REL doesn't support type %s", identType.name()));
+              String.format(
+                  "ROLE_GROUP_REL doesn't support type %s or loading all fields",


Is it possible that ROLE_GROUP_REL does not support all fields?

Now, we don't support all fields. We can support fetch all fields in the next pull request.

What is the meaning of this error message, I cannot quite follow your meaning.

Fixed the error message, remove wrong message or loading all fields.

core/src/main/java/org/apache/gravitino/storage/relational/service/RoleMetaService.java

yuqi1129 · 2024-09-26T06:27:24Z

server/src/main/java/org/apache/gravitino/server/web/rest/ObjectRoleOperations.java

+
+@NameBindings.AccessControlInterfaces
+@Path("/metalakes/{metalake}/objects/{type}/{fullName}/roles")
+public class ObjectRoleOperations {


Can we rename it to MetadataObjectRoleOperations?

yuqi1129 · 2024-09-26T06:30:26Z

core/src/main/java/org/apache/gravitino/SupportsRelationOperations.java

+  }
+
+  /**
+   * List the entities according to a give entity in a specific relation.


typo: give-> given

jerryshao · 2024-09-26T06:33:10Z

clients/client-java/src/main/java/org/apache/gravitino/client/GravitinoClient.java

+   */
+  public String[] listRoleNamesByObject(MetadataObject object)
+      throws NoSuchMetalakeException, NoSuchMetadataObjectException {
+    return getMetalake().listRoleNamesByObject(object);


It is better to design the APIs like tags, list roles from different entity object.

Different from Tag API, role is related to authorization. Only we enable authorization, entity can list roles. So I prefer making a dedicated API to do this. WDYT?

Do you honor authorization here? I don't see you honor the authorization here even you put the code here.

OK, I can refer to the Tag to modify here.

Think twice. Different from tag, tag don't support metalake type. role supportsmetalake.
If we add listObjectRoleNames API. It may make users confusing.
What's the difference between listObjectRoleNames and listRoleNames for a metalake.
If it's ok, I can modify this logic.

We don't want to expose GravitinoMetalake. If we want to list roles for GravitinoMetalake, it's hard.

How about using different method name like "listBindingRoleNames"?

You can add more descriptions to tell about the difference.

OK, I modified this code according to your suggestion.

jerryshao · 2024-09-26T11:44:39Z

...rc/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java

+
+    // List roles by the object
+    roleNames = metalake.listBindingRoleNames();
+    Arrays.sort(roleNames);


Can you test list roles for different objects other than metalake?

Added the test cases of catalog, schema and fileset.

jerryshao · 2024-09-26T13:02:12Z

clients/client-java/src/main/java/org/apache/gravitino/client/MetadataObjectRoleOperations.java

+
+  private final RESTClient restClient;
+
+  private final String tagRequestPath;


The variable name here is not correct.

jerryshao · 2024-09-26T13:16:59Z

server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectRoleOperations.java

+          httpRequest,
+          () ->
+              TreeLockUtils.doWithTreeLock(
+                  NameIdentifier.of(metalake),


You'd better lock the metadata object, not the metalake here.

yuqi1129 · 2024-09-26T13:12:39Z

api/src/main/java/org/apache/gravitino/authorization/SupportsRoles.java

+import org.apache.gravitino.annotation.Evolving;
+
+/**
+ * Interface for supporting list role names for objects. This interface will be mixed with metadata


be combined

SupportsTags use similar grammar.

yuqi1129 · 2024-09-26T13:15:41Z

api/src/main/java/org/apache/gravitino/authorization/SupportsRoles.java

+  /**
+   * List all the role names associated with this metadata object. For the metalake metadata object,
+   * this method will only return the roles contains metalake object instead of all the roles in the
+   * metalake.


For the metalake metadata object,

this method will only return the roles contains metalake object instead of all the roles in the

metalake.

Can you clarify it? I can't get the meaning.

Removed confusing comment.

yuqi1129 · 2024-09-26T13:17:00Z

api/src/main/java/org/apache/gravitino/authorization/SupportsRoles.java

+   *
+   * @return The role name list associated with this metadata object.
+   */
+  String[] listBindingRoleNames();


Why not give it a default implementation and throw UnsupportUnsupportedOperationException?

supportsRoles has throw UnSupportedOperationExcetion. supportsTags doesn't throw UnSupportedOperationException, too.

server/src/main/java/org/apache/gravitino/server/web/rest/MetadataObjectRoleOperations.java

jerryshao · 2024-09-27T06:04:44Z

core/src/main/java/org/apache/gravitino/storage/relational/service/RoleMetaService.java

-                POConverters.fromRolePO(
-                    po, listSecurableObjects(po), AuthorizationUtils.ofRoleNamespace(metalake)))
+            po -> {
+              if (allFields) {


When will you set this to true for list roles by metadata object?

This is used for internal implement. I used it in the FutureGrantManager, I need find all the roles contains metalake metadata object. For example, I find the role contains CREATE TABLE for a metalake and apply the role for newly created catalog.

jerryshao · 2024-09-27T06:09:13Z

core/src/main/java/org/apache/gravitino/storage/relational/service/RoleMetaService.java

+            POConverters.fromSecurableObjectPO(
+                fullName, securableObjectPO, getType(securableObjectPO.getType())));
+      } else {
+        LOG.info(


Can you please change to the warning log.

Besides, looks like if the object is deleted, then we will always get null result, maybe we should also delete the relation to role when we delete the object.

Besides, looks like if the object is deleted, then we will always get null result, maybe we should also delete the relation to role when we delete the object.

I have created an issue #5029 to track it.

### What changes were proposed in this pull request? Supports to list roles by object ### Why are the changes needed? Fix: #4886 ### Does this PR introduce _any_ user-facing change? I will add the document later. ### How was this patch tested? Add new UT.

jerqi force-pushed the ISSUE-4886 branch 2 times, most recently from c089126 to 447ae0a Compare September 26, 2024 03:11

Supports to list roles by object

3ccc9d5

jerqi force-pushed the ISSUE-4886 branch from 447ae0a to 3ccc9d5 Compare September 26, 2024 03:24

jerqi requested review from yuqi1129 September 26, 2024 03:54

jerqi self-assigned this Sep 26, 2024

yuqi1129 reviewed Sep 26, 2024

View reviewed changes

jerryshao reviewed Sep 26, 2024

View reviewed changes

jerqi added 2 commits September 26, 2024 15:09

address the comments

92be694

fix style

dc2a910

jerqi changed the title ~~[#4886] feat(server, core): Supports to list roles by object~~ [#4886] feat(server,core): Supports to list roles by object Sep 26, 2024

jerqi added the branch-0.6 Automatically cherry-pick commit to branch-0.6 label Sep 26, 2024

jerqi added 3 commits September 26, 2024 17:01

Modify clients

00d31a2

add comment

ea048c8

add license

130f517

jerryshao reviewed Sep 26, 2024

View reviewed changes

Add more tests

cda140b

jerryshao reviewed Sep 26, 2024

View reviewed changes

jerqi added 2 commits September 26, 2024 21:05

Fix varaible name

51014d5

correct error message

2ad0607

jerryshao reviewed Sep 26, 2024

View reviewed changes

yuqi1129 reviewed Sep 26, 2024

View reviewed changes

jerqi added 4 commits September 26, 2024 21:21

add the lock

f0dfc54

remove confusing comment

9de9732

rename

79570fd

correct comment

e93787d

jerryshao reviewed Sep 27, 2024

View reviewed changes

fix the log

eefdf8d

jerryshao approved these changes Sep 27, 2024

View reviewed changes

jerryshao merged commit dab92a5 into apache:main Sep 27, 2024
26 checks passed


		private final RESTClient restClient;

		private final String tagRequestPath;

[#4886] feat(server,core): Supports to list roles by object #5023

[#4886] feat(server,core): Supports to list roles by object #5023

Conversation

jerqi commented Sep 26, 2024

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

jerqi Sep 26, 2024 • edited Loading

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

jerqi Sep 26, 2024 • edited Loading

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

jerqi Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

jerryshao Sep 26, 2024 • edited Loading

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

jerqi Sep 26, 2024 •

edited

Loading

jerqi Sep 26, 2024 •

edited

Loading

jerqi Sep 26, 2024 •

edited

Loading

jerryshao Sep 26, 2024 •

edited

Loading