Copilot commented on code in PR #9384:
URL: https://github.com/apache/gravitino/pull/9384#discussion_r2589186216
##########
api/src/main/java/org/apache/gravitino/authorization/Privilege.java:
##########
@@ -89,9 +89,13 @@ enum Name {
CREATE_ROLE(0L, 1L << 16),
/** The privilege to grant or revoke a role for the user or the group. */
MANAGE_GRANTS(0L, 1L << 17),
- /** The privilege to create a model */
+ /** The privilege to register a model */
+ REGISTER_MODEL(0L, 1L << 18),
+ /** The privilege to create a model. The is deprecated. Please use
REGISTER_MODEL */
CREATE_MODEL(0L, 1L << 18),
- /** The privilege to create a model version */
+ /** The privilege to link a model version */
+ LINK_MODEL_VERSION(0L, 1L << 19),
+ /** The privilege to create a model version. This deprecated. Please use
LINK_MODEL_VERSION */
Review Comment:
Corrected grammar: 'This deprecated' should be 'This is deprecated'.
##########
clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/AccessControlIT.java:
##########
@@ -277,19 +278,33 @@ void testManageRoles() {
SecurableObjects.ofModel(
schemaObjectWithModelPrivs,
"model",
- Lists.newArrayList(Privileges.CreateModelVersion.allow(),
Privileges.UseModel.allow()));
+ Lists.newArrayList(Privileges.LinkModelVersion.allow(),
Privileges.UseModel.allow()));
role =
metalake.createRole(
"model_name", properties,
Lists.newArrayList(modelObjectWithCorrectPriv));
Assertions.assertEquals("model_name", role.name());
Assertions.assertEquals(properties, role.properties());
metalake.deleteRole("model_name");
+ // Test legacy privilege name
+ SecurableObject modelObjectWithLegacyPriv =
+ SecurableObjects.ofModel(
+ schemaObjectWithModelPrivs,
+ "model",
+ Lists.newArrayList(
+ Privileges.CreateModelVersion.allow(),
Privileges.CreateModelVersion.allow()));
Review Comment:
Duplicate privilege `Privileges.CreateModelVersion.allow()` in the list.
This appears to be a copy-paste error. The test should verify handling of
duplicate legacy privileges, but using the same privilege twice makes the test
less meaningful.
```suggestion
Privileges.CreateModelVersion.allow(),
Privileges.RegisterModel.allow()));
```
##########
api/src/main/java/org/apache/gravitino/authorization/Privilege.java:
##########
@@ -89,9 +89,13 @@ enum Name {
CREATE_ROLE(0L, 1L << 16),
/** The privilege to grant or revoke a role for the user or the group. */
MANAGE_GRANTS(0L, 1L << 17),
- /** The privilege to create a model */
+ /** The privilege to register a model */
+ REGISTER_MODEL(0L, 1L << 18),
+ /** The privilege to create a model. The is deprecated. Please use
REGISTER_MODEL */
Review Comment:
Corrected grammar: 'The is deprecated' should be 'This is deprecated'.
```suggestion
/** The privilege to create a model. This is deprecated. Please use
REGISTER_MODEL */
```
##########
docs/security/access-control.md:
##########
@@ -1025,7 +1025,7 @@ The following table lists the required privileges for
each API.
| drop fileset | First, you should have the privilege to
load the catalog and the schema. Then, you are one of the owners of the
fileset, schema, catalog, metalake
|
| list fileset | First, you should have the privilege to
load the catalog and the schema. Then, you are one of the owners of the schema,
catalog, metalake can see all the filesets, others can see the filesets which
they can load |
| load fileset | First, you should have the privilege to
load the catalog and the schema. Then, you are one of the owners of the
fileset, schema, metalake, catalog or have either `READ_FILESET` or
`WRITE_FILESET` on the fileset, schema, catalog, metalake |
-| list file | First, you should have the privilege to
load the catalog and the schema. Then, you are one of the owners of the
fileset, schema, metalake, catalog or have either `READ_FILESET` or
`WRITE_FILESET` on the fileset, schema, catalog, metalake |
+| list fileset | First, you should have the privilege to
load the catalog and the schema. Then, you are one of the owners of the
fileset, schema, metalake, catalog or have either `READ_FILESET` or
`WRITE_FILESET` on the fileset, schema, catalog, metalake |
| register model | First, you should have the privilege to
load the catalog and the schema. Then, you have `CREATE_MODEL` on the metalake,
catalog, schema or are the owner of the metalake, catalog, schema
|
| link model version | First, you should have the privilege to
load the catalog, the schema and the model. Then, you have
`CREATE_MODEL_VERSION` on the metalake, catalog, schema, model or are the owner
of the metalake, catalog, schema, model |
Review Comment:
Documentation still references the old privilege names `CREATE_MODEL` and
`CREATE_MODEL_VERSION` instead of the new names `REGISTER_MODEL` and
`LINK_MODEL_VERSION`. These should be updated to match the privilege renaming
in this PR.
```suggestion
| register model | First, you should have the privilege
to load the catalog and the schema. Then, you have `REGISTER_MODEL` on the
metalake, catalog, schema or are the owner of the metalake, catalog, schema
|
| link model version | First, you should have the privilege
to load the catalog, the schema and the model. Then, you have
`LINK_MODEL_VERSION` on the metalake, catalog, schema, model or are the owner
of the metalake, catalog, schema, model |
```
##########
core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java:
##########
@@ -229,7 +229,26 @@ public static void
checkDuplicatedNamePrivilege(Collection<Privilege> privileges
"Doesn't support duplicated privilege name %s with different
condition",
privilege.name());
}
- privilegeNameSet.add(privilege.name());
+ privilegeNameSet.add(replaceLegacyPrivilegeName(privilege.name()));
+ }
+ }
+
+ public static Privilege.Name replaceLegacyPrivilegeName(Privilege.Name
privilegeName) {
+ if (privilegeName == Privilege.Name.CREATE_MODEL) {
+ return Privilege.Name.REGISTER_MODEL;
+ } else if (privilegeName == Privilege.Name.CREATE_MODEL_VERSION) {
+ return Privilege.Name.LINK_MODEL_VERSION;
+ } else {
+ return privilegeName;
+ }
+ }
+
+ public static Privilege replaceLegacyPrivilege(
+ Privilege.Name privilege, Privilege.Condition condition) {
+ if (condition == Privilege.Condition.ALLOW) {
+ return Privileges.allow(privilege.name());
+ } else {
+ return Privileges.deny(privilege.name());
Review Comment:
This method uses `privilege.name()` (a String representation) instead of
passing the `privilege` enum directly. Since the `Privileges.allow()` method
has overloads for both String and Privilege.Name enum, this should pass the
enum value directly after replacement. The method should call
`replaceLegacyPrivilegeName(privilege)` first, then pass that result to
`Privileges.allow()` and `Privileges.deny()`.
```suggestion
Privilege.Name replacedPrivilege = replaceLegacyPrivilegeName(privilege);
if (condition == Privilege.Condition.ALLOW) {
return Privileges.allow(replacedPrivilege);
} else {
return Privileges.deny(replacedPrivilege);
```
--
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]
