[#2235][part-1] feat(api,core): Add the role entity #2772

qqqttt123 · 2024-04-02T07:02:39Z

What changes were proposed in this pull request?

This pull request adds the role entity and one privilege.
I avoid to create a large pull request. I will add more privileges in the later pull request.

Why are the changes needed?

Fix: #2235

Does this PR introduce any user-facing change?

No.

How was this patch tested?

new ut.

qqqttt123 · 2024-04-02T09:21:41Z

api/src/main/java/com/datastrato/gravitino/authorization/Privilege.java

+  String simpleString();
+
+  /** The name of this privilege. */
+  enum Name {


We only list parts of privileges to avoid a too large pull request. The extra privileges will be submitted by another pull request.

core/src/main/java/com/datastrato/gravitino/authorization/RoleManager.java

meta/src/main/proto/gravitino_meta.proto

core/src/main/java/com/datastrato/gravitino/authorization/AccessControlManager.java

This reverts commit 43a358c.

jerryshao · 2024-04-15T05:57:58Z

api/src/main/java/com/datastrato/gravitino/authorization/Resource.java

+ * The resource is the entity of the authorization. Gravitino organizes the resources using tree
+ * structure. The resource may be a catalog, a table or a schema, etc. For example,
+ * `catalog1.schema1.table1` represents a table named `table1`. It's in the schema named `schema1`.
+ * The schema is in the catalog named `catalog1`.


How do you represent the resources with name "*", would you please add more comments about the different types of resources and how you define them?

I moved some comments from other places.

jerryshao · 2024-04-15T06:04:04Z

api/src/main/java/com/datastrato/gravitino/authorization/Resource.java

+   *
+   * @return The parent resource.
+   */
+  Resource parent();


Can you tag this method as @Nullable?

jerryshao · 2024-04-15T06:08:10Z

api/src/main/java/com/datastrato/gravitino/authorization/Resources.java

+                + " If you want to create an another resource which represents all entities,"
+                + " you can use its parent entity, For example,"
+                + " if you want to have read table privileges of all tables of `catalog1.schema1`,"
+                + " you can use add `read table` privilege for `catalog1.schema1` directly");


Frankly saying, It's weird to add table privilege to the schema, it will confuse the users. Why do you hesitate to use "*"?

Now, we give a resource a privileges list. It's a little weird to add load catalog and load schema privilege to catalog1.*.

Databricks follows similar style. Parent doesn't use *. https://docs.databricks.com/en/sql/language-manual/sql-ref-privileges.html

jerryshao · 2024-04-15T06:14:33Z

api/src/main/java/com/datastrato/gravitino/authorization/Resources.java

+   * @param names The names of the resource
+   * @return The created {@link Resource}
+   */
+  public static Resource of(String... names) {


How do users use this Resource concept, what is the relationship to the catalogs, schemas, and tables? I think the introduction of Resource increase the understanding burden to users, users may not know how to use it, I was thinking if there's a better way to clearly define it, which will be easy for users to use.

How about letting me user pass the names like
ofCatalog. ofCatalog.ofSchema.

Databricks has the similar concept securable objects .
Snowflake use securable object, too. I will change the name to securable object.

jerryshao · 2024-04-15T06:18:12Z

core/src/main/java/com/datastrato/gravitino/catalog/CatalogManager.java

@@ -292,6 +292,10 @@ public Catalog createCatalog(
      throw new IllegalArgumentException("Can't create a catalog with with reserved name `system`");
    }

+    if (Entity.RESOURCE_ENTITY_RESERVED_NAME.equals(ident.name())) {


I think it is the basic name constraint for entities, better to let @mchades know about this and do it using Capability framework.

jerryshao · 2024-04-15T09:54:05Z

api/src/main/java/com/datastrato/gravitino/authorization/SecurableObjects.java

+    @Override
+    public String toString() {
+      if (parent != null) {
+        return parent + "." + name;


Can you explicitly write like parent.toString() + "." + name to let others know that this method is recursive?

jerryshao

LGTM.

qqqttt123 marked this pull request as draft April 2, 2024 07:02

qqqttt123 mentioned this pull request Apr 2, 2024

[Subtask] Add relational backend for User Entity #2705

Closed

qqqttt123 force-pushed the ISSUE-2235_feat branch 4 times, most recently from 3516475 to c76cd2e Compare April 2, 2024 09:20

qqqttt123 commented Apr 2, 2024

View reviewed changes

core/src/main/java/com/datastrato/gravitino/authorization/RoleManager.java Outdated Show resolved Hide resolved

qqqttt123 commented Apr 2, 2024

View reviewed changes

meta/src/main/proto/gravitino_meta.proto Outdated Show resolved Hide resolved

qqqttt123 force-pushed the ISSUE-2235_feat branch 6 times, most recently from d567611 to 65a1916 Compare April 2, 2024 10:40

qqqttt123 commented Apr 2, 2024

View reviewed changes

meta/src/main/proto/gravitino_meta.proto Outdated Show resolved Hide resolved

qqqttt123 force-pushed the ISSUE-2235_feat branch 2 times, most recently from 5e1f6b1 to 7715876 Compare April 2, 2024 11:40

qqqttt123 changed the title ~~[WIP] Add the role entity~~ [#2235][WIP] feat(core): Add the role entity Apr 7, 2024

qqqttt123 changed the title ~~[#2235][WIP] feat(core): Add the role entity~~ [#2235][WIP] feat(api,core): Add the role entity Apr 7, 2024

qqqttt123 force-pushed the ISSUE-2235_feat branch from 7715876 to 748ffbb Compare April 7, 2024 12:43

qqqttt123 commented Apr 7, 2024

View reviewed changes

core/src/main/java/com/datastrato/gravitino/authorization/AccessControlManager.java Show resolved Hide resolved

qqqttt123 changed the title ~~[#2235][WIP] feat(api,core): Add the role entity~~ [#2235][part-1] feat(api,core): Add the role entity Apr 7, 2024

qqqttt123 self-assigned this Apr 7, 2024

qqqttt123 requested a review from jerryshao April 7, 2024 12:52

qqqttt123 force-pushed the ISSUE-2235_feat branch from 748ffbb to bd564ac Compare April 7, 2024 13:07

qqqttt123 marked this pull request as ready for review April 7, 2024 13:07

[apache#2235] feat(api,core): Add the Role entity

83d897e

qqqttt123 force-pushed the ISSUE-2235_feat branch from bd564ac to 83d897e Compare April 7, 2024 13:12

Merge remote-tracking branch 'upstream/main' into ISSUE-2235_feat

db30184

Heng Qin added 9 commits April 13, 2024 12:01

fix

886322b

polish

b2c0324

final

7a09b57

polish comment

9a1404a

polish

c7ad2b2

User group client

43a358c

Add comment to explan the difference between resource and entity

cc697ce

Revert "User group client"

2ce3c2f

This reverts commit 43a358c.

polish the comment

e783d03

qqqttt123 force-pushed the ISSUE-2235_feat branch from f2d5a18 to 46e2879 Compare April 13, 2024 14:56

remove identifier

03c627f

qqqttt123 force-pushed the ISSUE-2235_feat branch from 46e2879 to 03c627f Compare April 13, 2024 15:15

Heng Qin added 2 commits April 15, 2024 11:29

fix typo

b5b3b69

fix

967b8b3

jerryshao reviewed Apr 15, 2024

View reviewed changes

Heng Qin added 3 commits April 15, 2024 15:52

Rename to securable object

6beb424

Add an annotation

7c29aad

fix left names

7b531c1

qqqttt123 requested a review from jerryshao April 15, 2024 09:21

Heng Qin added 2 commits April 15, 2024 17:37

add comment

b30f34a

comment

54c3161

jerryshao reviewed Apr 15, 2024

View reviewed changes

Heng Qin added 2 commits April 15, 2024 18:21

Add helper methods

f7ba2b8

toString

7765b38

jerryshao approved these changes Apr 15, 2024

View reviewed changes

jerryshao merged commit 64bf339 into apache:main Apr 15, 2024
22 checks passed

qqqttt123 deleted the ISSUE-2235_feat branch April 15, 2024 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#2235][part-1] feat(api,core): Add the role entity #2772

[#2235][part-1] feat(api,core): Add the role entity #2772

qqqttt123 commented Apr 2, 2024 •

edited

Loading

qqqttt123 Apr 2, 2024 •

edited

Loading

jerryshao Apr 15, 2024

qqqttt123 Apr 15, 2024

jerryshao Apr 15, 2024

qqqttt123 Apr 15, 2024

qqqttt123 Apr 15, 2024

jerryshao Apr 15, 2024 •

edited

Loading

qqqttt123 Apr 15, 2024 •

edited

Loading

qqqttt123 Apr 15, 2024

jerryshao Apr 15, 2024

qqqttt123 Apr 15, 2024

qqqttt123 Apr 15, 2024

qqqttt123 Apr 15, 2024

jerryshao Apr 15, 2024

jerryshao Apr 15, 2024

qqqttt123 Apr 15, 2024

jerryshao left a comment

[#2235][part-1] feat(api,core): Add the role entity #2772

[#2235][part-1] feat(api,core): Add the role entity #2772

Conversation

qqqttt123 commented Apr 2, 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?

qqqttt123 Apr 2, 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

jerryshao Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

qqqttt123 Apr 15, 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

jerryshao left a comment

Choose a reason for hiding this comment

qqqttt123 commented Apr 2, 2024 •

edited

Loading

qqqttt123 Apr 2, 2024 •

edited

Loading

jerryshao Apr 15, 2024 •

edited

Loading

qqqttt123 Apr 15, 2024 •

edited

Loading