[#2967] feat(api,server): Add the more privileges #2965

qqqttt123 · 2024-04-16T07:20:46Z

What changes were proposed in this pull request?

Add the more privileges and check the privilege constraint about securable object.

Why are the changes needed?

Fix: #2967

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add new UT.

qqqttt123 · 2024-04-16T07:55:55Z

The logic of this pull request is simple, there exists duplicate code, although this is a little large, so I don't divide it into two pull requests.

jerryshao · 2024-04-17T05:18:02Z

Is it ready for reviewing? @qqqttt123

qqqttt123 · 2024-04-17T06:02:43Z

Is it ready for reviewing? @qqqttt123

Yes. It is ready.

jerryshao · 2024-04-17T06:44:53Z

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

+    /** The privilege to drop a table. */
+    DROP_TABLE,
+    /** The privilege to read a table. */
+    READ_TABLE,


Do we need both LOAD_XXX and READ/WRITE_XXX? How will user use them?

Also how do you distinguish whether the table has read or write privilege from Gravitino?

Yes, LOAD_TABLE is a meta data operation and the READ_TABLE is a data operation. They are different operations.

Gravitino doesn't disitinguish the privilege. User doesn't use Gravitino to read or write table.
Gravitino set the privileges of read/ write table to the underlying system. The underlying system will judge the privilege of read/write when the users access the underlying system.

jerryshao · 2024-04-17T06:54:27Z

server/src/main/java/com/datastrato/gravitino/server/web/rest/RoleOperations.java

@@ -33,12 +44,54 @@
 public class RoleOperations {
  private static final Logger LOG = LoggerFactory.getLogger(RoleOperations.class);

+  private static final String PRIVILEGE_ERROR_MSG = "%s shouldn't bind the privilege %s";
+  private static final String UNSUPPORTED_ERROR_MSG = "%s uses an unsupported catalog type";
+  private static final List<Privilege> CATALOG_PRIVILEGES =


As you mentioned before that catalog will have the schema privileges like create schema, how do you achieve this in this PR?

You can see the comment of checkSecurableObjectPrivileges

jerryshao · 2024-04-17T06:57:20Z

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

+    /** The privilege to read a topic. */
+    READ_TOPIC,
+    /** The privilege to write a topic. */
+    WRITE_TOPIC


The name here are too specific for metadata, users may not know it is, like LOAD? I think you'd better have a more suitable name for users easy to understand.

LOAD_TOPIC means that we read the meta data of topic while READ_TOPIC means that we read data of topic.

qqqttt123 · 2024-04-17T09:06:32Z

@jerryshao I refactor the code

Remove LOAD_TABLE,LOAD_TOPIC,LOAD_FILESET.
Refactor the comment and the logic of checking privileges.

qqqttt123 · 2024-04-17T10:27:04Z

The CI pipeline of python failed. It's unrelated to this pull request.

jerryshao · 2024-04-18T03:38:15Z

server/src/main/java/com/datastrato/gravitino/server/web/rest/RoleOperations.java

@@ -33,12 +44,76 @@
 public class RoleOperations {
  private static final Logger LOG = LoggerFactory.getLogger(RoleOperations.class);

+  private static final String UNSUPPORTED_ERROR_MSG = "%s is an unsupported catalog type";
+  private static final List<Privilege> CATALOG_PRIVILEGES_EXCEPT_FOR_LIST =


Why don't you put the code in core, what is the specific reason?

The check logic needs to get the type of catalog. We should use tree lock to get the catalog. We usually use the tree lock in the operation instead of core.

jerryshao · 2024-04-18T03:39:49Z

I think you should really polish the code to make your check logic more clear and easy-to-understand before it is ready to review.

qqqttt123 · 2024-04-18T03:47:01Z

I think you should really polish the code to make your check logic more clear and easy-to-understand before it is ready to review.

I have no idea to make this pull request more clear.
I have tried to add more comments and wrap more methods. It's not suitable to use state machine.

qqqttt123 · 2024-04-20T14:50:06Z

@jerryshao I have removed the check code and add more privileges.

jerryshao · 2024-04-21T01:18:01Z

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

+    /** The privilege to alter a catalog. */
+    ALTER_CATALOG,
+    /** The privilege to drop a catalog. */
+    DROP_CATALOG,


Maybe we can also add a bit to each privilege like 0x01, 0x10, so that later on when we have a privilege like ALL_PRIVILEGE that can be represented as a combination of several privileges, you can think of this.

jerryshao · 2024-04-21T01:18:53Z

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

+    /** The privilege to drop a schema. */
+    DROP_SCHEMA,
+    /** The privilege to show tables. */
+    SHOW_TABLE,


SHOW_TABLES or SHOW_TABLE, I think for SHOW_XXX, it should add "s", right?

From UI's view, to show resources tree. Maybe we don't need show metalakes, show tables, show schemas privilege. Every one should have the privilege. I want to remove them a little.

Em...OK, I will correct this. My concern is that we don't use LOAD_A_SCHEMA. But I really want to all list entities this.

jerryshao · 2024-04-21T01:20:35Z

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

+    USE_METALAKE,
+    /** The privilege to manage a metalake. */
+    MANAGE_METALAKE,
+    /** The privilege to create a metalake. */
+    CREATE_METALAKE,
+    /** The privilege to manage users. */
+    MANAGE_USER,
+    /** The privilege to manage groups. */
+    MANAGE_GROUP,
+    /** The privilege to manage roles. */
+    MANAGE_ROLE,


These privileges should be explained well in the document and comment.

jerryshao · 2024-04-21T01:21:48Z

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

+   * A role is used for access control. A role will indicate that it has some privileges to access
+   * some securable objects.
+   */
+  ROLE


Why do we need a ROLE securable object?

We will create a special role for one entity.
One creator of one entity should have the privilege to grant the owner role to others.

Can you give an example? If it is user A who create a table "foo", so the privilege this "grant role", the securable object is table "foo", grant to user A, am I right? the securable object is table, not role, right?

That's ok, too. I will remove the entity.

jerryshao · 2024-04-21T01:22:32Z

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

+ * The type of securable object in the Gravitino system. Every type will map one kind of the entity
+ * of the underlying system.
+ */
+public enum SecurableObjectType {


I think these Type enum class can be an inner class in SecurableObject.

jerryshao · 2024-04-21T01:25:47Z

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

+    List<SecurableObjectType> typeInDifferentLevels =
+        Lists.newArrayList(SecurableObjectType.CATALOG, SecurableObjectType.SCHEMA);
+
+    if (names.length == 1 && type != SecurableObjectType.CATALOG) {


For metalake, the length should also be 1?

I will add some test code.

jerryshao · 2024-04-21T01:26:11Z

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

+    if (names.length > 2) {
+      typeInDifferentLevels.add(type);
+    }


What's the meaning of these codes?

I refactor these code to make it more clear.

jerryshao · 2024-04-21T01:29:11Z

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

@@ -38,27 +76,39 @@ public static SecurableObject of(String... names) {
      if (name.equals("*")) {
        throw new IllegalArgumentException(
            "Cannot create a securable object with `*` name. If you want to use a securable object which represents all catalogs,"


all metalakes, not all catalogs.

I will modify the comment. I will explain ofMetalake and ofAllMetalakes.

jerryshao · 2024-04-21T01:33:12Z

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

@@ -144,16 +193,19 @@ private static void checkCatalog(SecurableObject catalog) {
    }
  }

-  private static final SecurableObject ALL_CATALOGS = new SecurableObjectImpl(null, "*");
+  private static final SecurableObject ALL_METALAKES =
+      new SecurableObjectImpl(null, "*", SecurableObjectType.CATALOG);


Is this type correct?

My fault. I will add some tests for this class.

qqqttt123 · 2024-04-21T06:02:59Z

@jerryshao All comments are addressed.

jerryshao · 2024-04-22T01:59:47Z

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

+    /** The privilege to manage groups, including add,remove and get a group. */
+    MANAGE_GROUP(1L << 23),
+    /** The privilege to manage roles, including create,drop,alter,grant and revoke a role. */
+    MANAGE_ROLE(1L << 24);


I'm afraid that 64 bit may not be enough when we continue to add more fine-grained privileges, what do you think?

jerryshao · 2024-04-22T02:04:43Z

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

+ * represent a fileset.<br>
+ * `*` can represent all th metalakes <br>
+ * `metalake` can represent all the catalogs of this metalake. <br>
+ * To use other securable objects which represents all entities," you can use their parent entity,


I would suggest you add more descriptions and examples about this securable object.

I refactored the comment.

jerryshao · 2024-04-22T02:15:58Z

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

+    for (int parentNum = names.length - 2; parentNum >= 0; parentNum--) {
+      curType = getParentSecurableObjectType(curType);
+      types.add(curType);
+    }


This part of code is a bit of hardcoded, can you please refine this code and remove this hardcoded numbers.

jerryshao · 2024-04-22T02:24:13Z

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

   * @return The created {@link SecurableObject}
   */
-  public static SecurableObject parse(String securableObjectIdentifier) {
+  public static SecurableObject parse(String securableObjectIdentifier, SecurableObject.Type type) {


It's a bit unknown about this securableObjectIdentifier, so I would suggest not introducing new name/concept, you can use "fullName" and add more comments.

jerryshao · 2024-04-22T02:27:23Z

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

   * @param names The names of the securable object.
   * @return The created {@link SecurableObject}
   */
-  public static SecurableObject of(String... names) {
+  public static SecurableObject of(SecurableObject.Type type, String... names) {


This names maybe errorprone, you require that names be ordered like catalog, schema, table, but in your code you cannot do such enforcement, so I would suggest you redefine your interface to avoid misuse.

I removed the public.

jerryshao · 2024-04-22T02:30:41Z

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

+    MANAGE_USER(1L << 22),
+    /** The privilege to manage groups, including add,remove and get a group. */
+    MANAGE_GROUP(1L << 23),
+    /** The privilege to manage roles, including create,drop,alter,grant and revoke a role. */
+    MANAGE_ROLE(1L << 24);


I would suggest we use fine-grained privilege for USER, GROUP, ROLE.

qqqttt123 · 2024-04-22T06:54:06Z

@jerryshao All comments are addressed.

jerryshao · 2024-04-22T12:38:38Z

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

+ * `SecurableObjects.ofTopic("topic1")` to create the securable object, or you can use full name
+ * `catalog1.schema1.topic1` and type `TOPIC` in the RESTFUL API. <br>
+ * If you want to use a fileset named `fileset1` in the schema named `schema1`, you can use the code
+ * `SecurableObjects.ofFileset("fileset1)` to create the securable object, or you can use full name


I think it is different the API you offered, you miss the parent securable object.

qqqttt123 · 2024-04-22T13:45:38Z

I test this pull request with the commands as follows;

curl -X POST -H "Accept: application/vnd.gravitino.v1+json" \
-H "Content-Type: application/json" -d '{"name":"metalake1","privileges":["USE_CATALOG"], "securableObject":{"fullName": "catalog","type": "catalog"}}' \
http://localhost:8090/api/metalakes/metalake/roles
{"code":0,"role":{"name":"metalake1","audit":{"creator":"anonymous","createTime":"2024-04-22T13:44:24.839Z"},"privileges":["USE_CATALOG"],"securableObject":{"fullName":"catalog","type":"catalog"}}}%

It's ok.

### What changes were proposed in this pull request? Add the more privileges and check the privilege constraint about securable object. ### Why are the changes needed? Fix: apache#2967 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Add new UT. --------- Co-authored-by: Heng Qin <qqtt@123.com>

qqqttt123 self-assigned this Apr 16, 2024

qqqttt123 marked this pull request as draft April 16, 2024 07:20

qqqttt123 force-pushed the ISSUE-2235-part2 branch 2 times, most recently from 1f48ae3 to c891960 Compare April 16, 2024 07:41

qqqttt123 changed the title ~~[#2235][part-2] feat(api,server): Add the more privileges~~ [#2968] feat(api,server): Add the more privileges Apr 16, 2024

qqqttt123 force-pushed the ISSUE-2235-part2 branch from c891960 to f1231a7 Compare April 16, 2024 07:45

qqqttt123 changed the title ~~[#2968] feat(api,server): Add the more privileges~~ [#2967] feat(api,server): Add the more privileges Apr 16, 2024

qqqttt123 marked this pull request as ready for review April 16, 2024 08:02

qqqttt123 requested a review from jerryshao April 16, 2024 08:03

qqqttt123 force-pushed the ISSUE-2235-part2 branch from f1231a7 to f75aa10 Compare April 16, 2024 08:15

jerryshao reviewed Apr 17, 2024

View reviewed changes

qqqttt123 requested a review from jerryshao April 17, 2024 09:06

jerryshao reviewed Apr 18, 2024

View reviewed changes

qqqttt123 force-pushed the ISSUE-2235-part2 branch 2 times, most recently from 59773bc to 6763ffa Compare April 20, 2024 06:03

Heng Qin added 6 commits April 20, 2024 15:17

[apache#2235][part-2] feat(api,server): Add the more privileges

a92be52

address comments

28bbcae

Address comments

5c90f07

address comment

e5da58e

address comment

343c709

fix style

659573a

Heng Qin added 5 commits April 20, 2024 21:31

fix

cf02896

fix priv

0ee8113

revert

d55b54e

revert little test

a5d9ade

revert

f4a88f4

jerryshao reviewed Apr 21, 2024

View reviewed changes

Heng Qin added 4 commits April 21, 2024 11:26

Address comments

b18d1c2

address comments

9eb0620

address comment

007c407

fix

063c8df

Heng Qin added 3 commits April 21, 2024 14:42

remove public

90fa539

rename

4270e90

typo

91bb2b0

qqqttt123 force-pushed the ISSUE-2235-part2 branch from 73b7d97 to 91bb2b0 Compare April 22, 2024 01:49

jerryshao reviewed Apr 22, 2024

View reviewed changes

Heng Qin added 2 commits April 22, 2024 14:21

address comments

ae7e3f9

fix ut

9c66bcf

qqqttt123 requested a review from jerryshao April 22, 2024 06:57

jerryshao reviewed Apr 22, 2024

View reviewed changes

Heng Qin added 3 commits April 22, 2024 20:54

fix

7bb72ef

address comment

23a3d40

address comment

28414cf

qqqttt123 requested a review from jerryshao April 22, 2024 13:13

jerryshao approved these changes Apr 22, 2024

View reviewed changes

qqqttt123 merged commit 2a1ba00 into apache:main Apr 22, 2024
22 checks passed

[#2967] feat(api,server): Add the more privileges #2965

[#2967] feat(api,server): Add the more privileges #2965

Conversation

qqqttt123 commented Apr 16, 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 commented Apr 16, 2024

jerryshao commented Apr 17, 2024

qqqttt123 commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqqttt123 Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqqttt123 commented Apr 17, 2024

qqqttt123 commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao commented Apr 18, 2024

qqqttt123 commented Apr 18, 2024

qqqttt123 commented Apr 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqqttt123 Apr 21, 2024 • edited Loading

Choose a reason for hiding this comment

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

qqqttt123 commented Apr 21, 2024

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

qqqttt123 commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqqttt123 commented Apr 22, 2024

qqqttt123 commented Apr 16, 2024 •

edited

Loading

qqqttt123 Apr 17, 2024 •

edited

Loading

jerryshao Apr 17, 2024 •

edited

Loading

qqqttt123 Apr 21, 2024 •

edited

Loading

qqqttt123 Apr 21, 2024 •

edited

Loading