[#3342] feat(core): Refactor the role and privilege model #3389

jerqi · 2024-05-14T11:26:36Z

What changes were proposed in this pull request?

Add support for ALLOW/DENY effect for privileges.
Move privileges from the role to the securable object.

Why are the changes needed?

Fix: #3342

Does this PR introduce any user-facing change?

Yes.

How was this patch tested?

UT passed.

jerryshao · 2024-05-16T03:37:59Z

@jerqi is this ready for review?

jerqi · 2024-05-16T04:01:53Z

https://docs.starburst.io/latest/security/biac-privileges.html

jerryshao · 2024-05-16T03:48:44Z

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

+   * means that you are denied to use the privilege. If you have `ALLOW` and `DENY` for the same
+   * privilege name of the same securable object, the `DENY` will take effect.
+   */
+  enum Effect {


What do other systems like Ranger describe this behavior, do they use Effect or other better word?

Gravitiy use the word Effect in their api.

Ranger use Allow conditions and Deny conditions.

I prefer to use "Condition" than "Effect".

jerryshao · 2024-05-16T03:54:54Z

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

   *
   * @param privilege The string representation of the privilege.
   * @return The Privilege.
   */
-  public static Privilege fromString(String privilege) {
+  public static Privilege allowPrivilegeFromString(String privilege) {


I think the method name is a little verbose, maybe we can change to like:

public Privilege allow(String name); public Privilege allow(Privilege.Name name); public Privilege deny(String name); public Privilege deny(Privilege.Name name);

jerryshao · 2024-05-16T07:16:25Z

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

+   *
+   * @return The privileges of the role.
+   */
+  List<Privilege> privileges();


Are these privileges inheritable in your current design?

They are not inheritable.

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

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

jerryshao · 2024-05-16T07:37:20Z

common/src/main/java/com/datastrato/gravitino/dto/authorization/SecurableObjectDTO.java

+  @Override
+  public void bindPrivileges(List<Privilege> privileges) {
+    this.privileges = privileges.stream().map(DTOConverters::toDTO).toArray(PrivilegeDTO[]::new);
+  }


The interface (securable object) you designed and achieved here in DTO is a bit weird, who and when will call this method?

It is because your SecurableObject has both getter and setter interfaces, it is mutable as the interface design. But DTO is immutable, so the interface design will make the DTO implementation a bit weird. I would suggest you rethink about how to design this SecurableObject based on our UI requirement as well as code itself.

jerryshao · 2024-05-16T07:39:40Z

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

@@ -250,21 +250,16 @@ public boolean isMetalakeAdmin(String user) {
   * @param role The name of the Role.
   * @param properties The properties of the Role.
   * @param securableObject The securable object of the Role.
-   * @param privileges The privileges of the Role.
   * @return The created Role instance.
   * @throws RoleAlreadyExistsException If a Role with the same name already exists.
   * @throws NoSuchMetalakeException If the Metalake with the given name does not exist.
   * @throws RuntimeException If creating the Role encounters storage issues.
   */
  public Role createRole(


Do we support add new securable object to the existing role?

Now, we don't support it. We can support it in the future.

jerryshao · 2024-05-16T07:43:21Z

meta/src/main/proto/gravitino_meta.proto

-  AuditInfo audit_info = 7;
+  repeated string privilege_effects = 4;
+  string securable_object_full_name = 5;
+  string securable_object_type = 6;


I would suggest you support multiple securable objects here, otherwise you need to update the storage structure later on, which is hard to do so.

In the meantime, since you changed the definition of securable object, should you also update the layout here?

Will you leave this storage part to the next PR?

Yes, I will leave the storage to the next PR.

qqqttt123 · 2024-05-20T12:01:51Z

After last review,

I remove bindPrivileges and make the interface immutable.
I change the effect to condition.

Could you give me further review? @jerryshao

jerryshao · 2024-05-21T03:09:11Z

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

+  /**
+   * The privileges of the securable object. For example: If the securable object is a table, the
+   * privileges could be `READ TABLE`, `WRITE TABLE`, etc. If a schema has the privilege of `LOAD
+   * TABLE`. It means the role can all tables of the schema.


“It means the role can all”？

jerryshao · 2024-05-21T03:16:29Z

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

-  public static SecurableObject ofSchema(SecurableObject catalog, String schema) {
-    checkCatalog(catalog);
+  public static SecurableObject ofSchema(
+      String catalogFullName, String schema, List<Privilege> privileges) {


What's the meaning of catalogFullName?

How do we represent the full name of the catalog, schema, etc, "a.b.c"? I think it will be better to use a class to restrict the format of the full name, otherwise, it is hard for users to formalize the input string.

I replace this by securable object.

jerryshao · 2024-05-21T04:01:52Z

clients/client-java/src/main/java/com/datastrato/gravitino/client/DTOConverters.java

+                .map(
+                    privilege -> {
+                      return PrivilegeDTO.builder()
+                          .withEffect(privilege.condition())


It should be changed to withCondition

jerryshao · 2024-05-21T04:04:52Z

common/src/main/java/com/datastrato/gravitino/dto/authorization/PrivilegeDTO.java

+  @JsonProperty("name")
+  private Name name;
+
+  @JsonProperty("effect")


This should also be updated.

jerryshao · 2024-05-21T04:06:07Z

meta/src/main/proto/gravitino_meta.proto

-  AuditInfo audit_info = 7;
+  repeated string privilege_effects = 4;
+  string securable_object_full_name = 5;
+  string securable_object_type = 6;


Will you leave this storage part to the next PR?

jerryshao · 2024-05-21T04:08:21Z

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


-    SecurableObjectImpl(SecurableObject parent, String name, Type type) {
-      this.parent = parent;
+    SecurableObjectImpl(String parentFullName, String name, Type type, List<Privilege> privileges) {


What is the purpose of using parentFullName instead of SecurableObject parent? How about using NameIdentifier for unification?

NameIdentifier will include the information of the metalake.

jerryshao · 2024-05-21T04:18:51Z

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


-    return new SecurableObjectImpl(null, metalake, SecurableObject.Type.METALAKE);
+    return new SecurableObjectImpl(null, metalake, SecurableObject.Type.METALAKE, privileges);


I think you can use SecurableObject.of(xxx), that has some checks before creating a securable object.

jerryshao · 2024-05-21T06:04:25Z

scripts/mysql/schema-0.5.0-mysql.sql

@@ -138,6 +138,7 @@ CREATE TABLE IF NOT EXISTS `role_meta` (
    `securable_object_full_name` VARCHAR(256) NOT NULL COMMENT 'securable object full name',
    `securable_object_type` VARCHAR(32) NOT NULL COMMENT 'securable object type',
    `privileges` VARCHAR(64) NOT NULL COMMENT 'securable privileges',
+    `privilege_conditions` VARCHAR(64) NOT NULL COMMENT 'securable privilege conditions',


Will you leave the multiple securable object support in the next pr?

Yes, I will

jerqi · 2024-05-21T10:14:12Z

@jerryshao Could you review again?

jerryshao · 2024-05-21T12:58:20Z

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

+        return GetRole.deny();
+
+      default:
+        throw new IllegalArgumentException("Don't support the privilege: " + name);


“Doesn't...”

jerryshao · 2024-05-21T14:43:29Z

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

      case GET_ROLE:
-        return GetRole.get();
+        return GetRole.allow();

      default:
        throw new IllegalArgumentException("Don't support the privilege: " + name);


“Doesn't support...”

jerryshao · 2024-05-21T14:48:07Z

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

   */
  @Nullable
-  SecurableObject parent();
+  String parentFullName();


Can we use String parent(), which seems simpler, WDYT?

OK, maybe it's enough for us to tell user this is a full name string.

qqqttt123 · 2024-05-22T01:06:09Z

@jerryshao Comments are addressed.

### What changes were proposed in this pull request? Add support for ALLOW/DENY effect for privileges. Move privileges from the role to the securable object. ### Why are the changes needed? Fix: #3342 ### Does this PR introduce _any_ user-facing change? Yes. ### How was this patch tested? UT passed. --------- Co-authored-by: Heng Qin <[email protected]>

…he#3389) ### What changes were proposed in this pull request? Add support for ALLOW/DENY effect for privileges. Move privileges from the role to the securable object. ### Why are the changes needed? Fix: apache#3342 ### Does this PR introduce _any_ user-facing change? Yes. ### How was this patch tested? UT passed. --------- Co-authored-by: Heng Qin <[email protected]>

jerqi marked this pull request as draft May 14, 2024 11:26

jerqi force-pushed the ISSUE-3342 branch from 537f481 to 8163833 Compare May 15, 2024 08:05

jerqi changed the title ~~[#3342] Refactor the role and privilege model~~ [#3342] feat(core): Refactor the role and privilege model May 15, 2024

jerqi force-pushed the ISSUE-3342 branch from 8163833 to f1a5741 Compare May 15, 2024 08:35

qqqttt123 assigned jerqi May 15, 2024

jerqi force-pushed the ISSUE-3342 branch 6 times, most recently from 612ee96 to 8828bf6 Compare May 15, 2024 12:22

[apache#3342] Refactor the role and privilege model

37bc98d

jerqi force-pushed the ISSUE-3342 branch from 8828bf6 to 37bc98d Compare May 15, 2024 12:36

qqqttt123 marked this pull request as ready for review May 15, 2024 13:09

jerryshao reviewed May 16, 2024

View reviewed changes

Heng Qin added 4 commits May 20, 2024 00:52

Address comments

097090c

refactor the comment

c620b13

Change SQL

2268bd6

Add constraint back

c19a26b

jerryshao reviewed May 21, 2024

View reviewed changes

Heng Qin added 2 commits May 21, 2024 16:03

Address comments

80a78c1

Address comments

adb8ee0

Heng Qin added 2 commits May 21, 2024 20:48

Rename condition

f37d114

Rename

ef3a68b

jerryshao reviewed May 21, 2024

View reviewed changes

address comments

70f932d

parentFullName -> parent

4a6b998

jerryshao added the branch-0.5 label May 22, 2024

jerryshao approved these changes May 22, 2024

View reviewed changes

jerryshao merged commit 5a4931b into apache:main May 22, 2024
22 checks passed


		return new SecurableObjectImpl(null, metalake, SecurableObject.Type.METALAKE);
		return new SecurableObjectImpl(null, metalake, SecurableObject.Type.METALAKE, privileges);

[#3342] feat(core): Refactor the role and privilege model #3389

[#3342] feat(core): Refactor the role and privilege model #3389

Conversation

jerqi commented May 14, 2024 • edited by qqqttt123 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?

jerryshao commented May 16, 2024

jerqi commented May 16, 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

Choose a reason for hiding this comment

qqqttt123 commented May 20, 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

jerqi commented May 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

qqqttt123 commented May 22, 2024

jerqi commented May 14, 2024 •

edited by qqqttt123

Loading

qqqttt123 commented May 20, 2024 •

edited

Loading