Copilot commented on code in PR #9177: URL: https://github.com/apache/gravitino/pull/9177#discussion_r2554827854
##########
api/src/main/java/org/apache/gravitino/authorization/SecurableObjects.java:
##########
@@ -138,6 +138,39 @@ public static SecurableObject ofModel(
return of(MetadataObject.Type.MODEL, names, privileges);
}
+ /**
+ * Create the tag {@link SecurableObject} with the given tag name and
privileges.
+ *
+ * @param tag The tag name
+ * @param privileges The privileges of the tag
+ * @return The created tag {@link SecurableObject}
+ */
+ public static SecurableObject ofTag(String tag, List<Privilege> privileges) {
+ return of(MetadataObject.Type.TAG, Lists.newArrayList(tag), privileges);
+ }
+
+ /**
+ * Create the policy {@link SecurableObject} with the given policy name and
privileges.
+ *
+ * @param policy The policy name
+ * @param privileges The privileges of the policy
+ * @return The created policy {@link SecurableObject}
+ */
+ public static SecurableObject ofPolicy(String policy, List<Privilege>
privileges) {
+ return of(MetadataObject.Type.POLICY, Lists.newArrayList(policy),
privileges);
+ }
+
+ /**
+ * Create the job {@link SecurableObject} with the given job name and
privileges.
Review Comment:
The JavaDoc comment doesn't match the implementation. The comment says
"Create the job {@link SecurableObject}" but the parameter name and
implementation create a job template, not a job. The comment should be updated
to: "Create the job template {@link SecurableObject} with the given job
template name and privileges."
```suggestion
* Create the job template {@link SecurableObject} with the given job
template name and privileges.
```
##########
api/src/main/java/org/apache/gravitino/authorization/Privileges.java:
##########
@@ -222,10 +237,22 @@ public static Privilege deny(Privilege.Name name) {
return CreateModelVersion.deny();
case USE_MODEL:
return UseModel.deny();
+
+ // Tag
case CREATE_TAG:
return CreateTag.deny();
case APPLY_TAG:
return ApplyTag.deny();
+
Review Comment:
Missing cases for APPLY_POLICY and CREATE_POLICY in the deny() method. These
privilege types are supported in the allow() method (lines 144-147) but are not
handled here. This would cause an IllegalArgumentException if someone tries to
use deny() for these privileges. Add:
```java
// Policy
case APPLY_POLICY:
return ApplyPolicy.deny();
case CREATE_POLICY:
return CreatePolicy.deny();
```
between the Tag section and Job template section.
```suggestion
// Policy
case APPLY_POLICY:
return ApplyPolicy.deny();
case CREATE_POLICY:
return CreatePolicy.deny();
```
##########
docs/security/access-control.md:
##########
@@ -1059,4 +1074,12 @@ The following table lists the required privileges for
each API.
| associate-object-policies | Requires both `APPLY_POLICY` permission
and permission to **load metadata objects**.
|
| list policies for object | Requires both permission to **get the
policy** and permission to **load metadata objects**.
|
| get policy for object | Requires both permission to **get the
policy** and permission to **load metadata objects**.
|
-
+| list job templates | The owner of the metalake can see all
the job templates, others can see the jobs which they can get.
|
+| register a job template | `CREATE_JOB_TEMPLATE` on the metalake or
the owner of the metalake.
|
+| get a job template | `USE_JOB_TEMPLATE` on the metalake or
job template, the owner of the metalake or the job template.
|
+| delete a job template | The owner of the metalake or the job
template
|
+| alter a job template | The owner of the metalake or the job
template
|
+| list jobs | The owner of the metalake can see all
the jobs, others can see the jobs which they can get.
|
+| run a job | The owner of the metalake , or have both
`RUN_JOB` on the metalake and `USE_JOB_TEMPLATE` on the job template
|
+| get a job | The owner of the metalake or the job
|
Review Comment:
Missing period at the end of the sentence. Should end with a period like the
other entries in the table.
```suggestion
| get a job | The owner of the metalake or the job.
|
```
##########
docs/security/access-control.md:
##########
@@ -1059,4 +1074,12 @@ The following table lists the required privileges for
each API.
| associate-object-policies | Requires both `APPLY_POLICY` permission
and permission to **load metadata objects**.
|
| list policies for object | Requires both permission to **get the
policy** and permission to **load metadata objects**.
|
| get policy for object | Requires both permission to **get the
policy** and permission to **load metadata objects**.
|
-
+| list job templates | The owner of the metalake can see all
the job templates, others can see the jobs which they can get.
|
Review Comment:
The text says "others can see the jobs which they can get" but this is
describing the "list job templates" operation. Should be "others can see the
job templates which they can get" for consistency.
```suggestion
| list job templates | The owner of the metalake can see all
the job templates, others can see the job templates which they can get.
|
```
##########
docs/security/access-control.md:
##########
@@ -1059,4 +1074,12 @@ The following table lists the required privileges for
each API.
| associate-object-policies | Requires both `APPLY_POLICY` permission
and permission to **load metadata objects**.
|
| list policies for object | Requires both permission to **get the
policy** and permission to **load metadata objects**.
|
| get policy for object | Requires both permission to **get the
policy** and permission to **load metadata objects**.
|
-
+| list job templates | The owner of the metalake can see all
the job templates, others can see the jobs which they can get.
|
+| register a job template | `CREATE_JOB_TEMPLATE` on the metalake or
the owner of the metalake.
|
+| get a job template | `USE_JOB_TEMPLATE` on the metalake or
job template, the owner of the metalake or the job template.
|
+| delete a job template | The owner of the metalake or the job
template
|
+| alter a job template | The owner of the metalake or the job
template
|
+| list jobs | The owner of the metalake can see all
the jobs, others can see the jobs which they can get.
|
+| run a job | The owner of the metalake , or have both
`RUN_JOB` on the metalake and `USE_JOB_TEMPLATE` on the job template
|
Review Comment:
Double space between "and" and "`USE_JOB_TEMPLATE`". Should be a single
space.
```suggestion
| run a job | The owner of the metalake , or have
both `RUN_JOB` on the metalake and `USE_JOB_TEMPLATE` on the job template
|
```
##########
docs/security/access-control.md:
##########
@@ -1059,4 +1074,12 @@ The following table lists the required privileges for
each API.
| associate-object-policies | Requires both `APPLY_POLICY` permission
and permission to **load metadata objects**.
|
| list policies for object | Requires both permission to **get the
policy** and permission to **load metadata objects**.
|
| get policy for object | Requires both permission to **get the
policy** and permission to **load metadata objects**.
|
-
+| list job templates | The owner of the metalake can see all
the job templates, others can see the jobs which they can get.
|
+| register a job template | `CREATE_JOB_TEMPLATE` on the metalake or
the owner of the metalake.
|
+| get a job template | `USE_JOB_TEMPLATE` on the metalake or
job template, the owner of the metalake or the job template.
|
+| delete a job template | The owner of the metalake or the job
template
|
Review Comment:
Missing period at the end of the sentence. Should end with a period like the
other entries in the table.
```suggestion
| delete a job template | The owner of the metalake or the job
template.
|
```
##########
docs/security/access-control.md:
##########
@@ -1059,4 +1074,12 @@ The following table lists the required privileges for
each API.
| associate-object-policies | Requires both `APPLY_POLICY` permission
and permission to **load metadata objects**.
|
| list policies for object | Requires both permission to **get the
policy** and permission to **load metadata objects**.
|
| get policy for object | Requires both permission to **get the
policy** and permission to **load metadata objects**.
|
-
+| list job templates | The owner of the metalake can see all
the job templates, others can see the jobs which they can get.
|
+| register a job template | `CREATE_JOB_TEMPLATE` on the metalake or
the owner of the metalake.
|
+| get a job template | `USE_JOB_TEMPLATE` on the metalake or
job template, the owner of the metalake or the job template.
|
+| delete a job template | The owner of the metalake or the job
template
|
+| alter a job template | The owner of the metalake or the job
template
|
Review Comment:
Missing period at the end of the sentence. Should end with a period like the
other entries in the table.
```suggestion
| alter a job template | The owner of the metalake or the job
template.
|
```
##########
docs/security/access-control.md:
##########
@@ -1059,4 +1074,12 @@ The following table lists the required privileges for
each API.
| associate-object-policies | Requires both `APPLY_POLICY` permission
and permission to **load metadata objects**.
|
| list policies for object | Requires both permission to **get the
policy** and permission to **load metadata objects**.
|
| get policy for object | Requires both permission to **get the
policy** and permission to **load metadata objects**.
|
-
+| list job templates | The owner of the metalake can see all
the job templates, others can see the jobs which they can get.
|
+| register a job template | `CREATE_JOB_TEMPLATE` on the metalake or
the owner of the metalake.
|
+| get a job template | `USE_JOB_TEMPLATE` on the metalake or
job template, the owner of the metalake or the job template.
|
+| delete a job template | The owner of the metalake or the job
template
|
+| alter a job template | The owner of the metalake or the job
template
|
+| list jobs | The owner of the metalake can see all
the jobs, others can see the jobs which they can get.
|
+| run a job | The owner of the metalake , or have both
`RUN_JOB` on the metalake and `USE_JOB_TEMPLATE` on the job template
|
+| get a job | The owner of the metalake or the job
|
+| cancel a job | The owner of the metalake or the job
|
Review Comment:
Missing period at the end of the sentence. Should end with a period like the
other entries in the table.
```suggestion
| cancel a job | The owner of the metalake or the job.
|
```
--
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]
