Copilot commented on code in PR #9283:
URL: https://github.com/apache/gravitino/pull/9283#discussion_r2568348684
##########
server/src/main/java/org/apache/gravitino/server/web/rest/JobOperations.java:
##########
@@ -329,8 +384,11 @@ public Response runJob(@PathParam("metalake") String
metalake, JobRunRequest req
@Path("runs/{jobId}")
@Produces("application/vnd.gravitino.v1+json")
@Timed(name = "cancel-job." + MetricNames.HTTP_PROCESS_DURATION, absolute =
true)
+ @AuthorizationExpression(expression = "METALAKE::OWNER || JOB::OWNER")
public Response cancelJob(
- @PathParam("metalake") String metalake, @PathParam("jobId") String
jobId) {
+ @PathParam("metalake") @AuthorizationMetadata(type =
Entity.EntityType.METALAKE)
+ String metalake,
+ @AuthorizationMetadata(type = Entity.EntityType.JOB) @PathParam("jobId")
String jobId) {
Review Comment:
[nitpick] The annotation order is unconventional. Place @PathParam before
@AuthorizationMetadata for better readability and consistency with other
parameters in this file.
```suggestion
@PathParam("jobId") @AuthorizationMetadata(type =
Entity.EntityType.JOB) String jobId) {
```
##########
core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java:
##########
@@ -190,21 +198,37 @@ public static void checkMetadataObject(String metalake,
MetadataObject object) {
try {
env.accessControlDispatcher().getRole(metalake, object.fullName());
} catch (NoSuchRoleException nsr) {
- Preconditions.checkArgument(
- exceptionToThrowSupplier != null, "exceptionToThrowSupplier
should not be null");
throw exceptionToThrowSupplier.get();
}
break;
+
case TAG:
NameIdentifierUtil.checkTag(identifier);
try {
env.tagDispatcher().getTag(metalake, object.fullName());
} catch (NoSuchTagException nsr) {
- Preconditions.checkArgument(
- exceptionToThrowSupplier != null, "exceptionToThrowSupplier
should not be null");
throw exceptionToThrowSupplier.get();
Review Comment:
Same issue as Comment 4: removed precondition check could lead to less
informative errors if `exceptionToThrowSupplier` is null.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/JobOperations.java:
##########
@@ -98,8 +106,19 @@ public Response listJobTemplates(
return Utils.doAs(
httpRequest,
() -> {
- List<JobTemplateDTO> jobTemplates =
-
toJobTemplateDTOs(jobOperationDispatcher.listJobTemplates(metalake));
+ List<JobTemplateEntity> jobTemplateEntities =
+ Lists.newArrayList(
+ MetadataAuthzHelper.filterByExpression(
+ metalake,
+ "METALAKE::OWNER || JOB_TEMPLATE::OWNER ||
METALAKE::USE_JOB_TEMPLATE || JOB_TEMPLATE::USE_JOB_TEMPLATE",
+ Entity.EntityType.JOB_TEMPLATE,
+ jobOperationDispatcher
+ .listJobTemplates(metalake)
+ .toArray(new JobTemplateEntity[0]),
+ jobTemplateEntity ->
+ NameIdentifierUtil.ofJobTemplate(metalake,
jobTemplateEntity.name())));
Review Comment:
Converting List to array and back to List is inefficient. Consider using a
different filtering approach or working directly with the collection type.
```suggestion
MetadataAuthzHelper.filterByExpression(
metalake,
"METALAKE::OWNER || JOB_TEMPLATE::OWNER ||
METALAKE::USE_JOB_TEMPLATE || JOB_TEMPLATE::USE_JOB_TEMPLATE",
Entity.EntityType.JOB_TEMPLATE,
jobOperationDispatcher.listJobTemplates(metalake),
jobTemplateEntity ->
NameIdentifierUtil.ofJobTemplate(metalake,
jobTemplateEntity.name()));
```
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/JobTemplateMetaMapper.java:
##########
@@ -70,4 +70,12 @@ Integer deleteJobTemplateMetasByLegacyTimeline(
Integer updateJobTemplateMeta(
@Param("newJobTemplateMeta") JobTemplatePO newJobTemplatePO,
@Param("oldJobTemplateMeta") JobTemplatePO oldJobTemplatePO);
+
+ @SelectProvider(
+ type = JobTemplateMetaSQLProviderFactory.class,
+ method = "selectJobTemplateIdByMetalakeAndName")
+ Long selectJobTemplateIdByMetalakeAndName(
+ @Param("metalakeId") long metalakeId, @Param("jobTemplateName") String
jobTemplateName);
+
+ String selectJobTemplateById(Long jobTemplateId);
Review Comment:
Missing @SelectProvider annotation or implementation. This method signature
appears incomplete - it should either have an annotation specifying how to
execute the query or be removed if not yet implemented.
```suggestion
@SelectProvider(
type = JobTemplateMetaSQLProviderFactory.class,
method = "selectJobTemplateById")
JobTemplatePO selectJobTemplateById(@Param("jobTemplateId") Long
jobTemplateId);
```
##########
core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java:
##########
@@ -190,21 +198,37 @@ public static void checkMetadataObject(String metalake,
MetadataObject object) {
try {
env.accessControlDispatcher().getRole(metalake, object.fullName());
} catch (NoSuchRoleException nsr) {
- Preconditions.checkArgument(
- exceptionToThrowSupplier != null, "exceptionToThrowSupplier
should not be null");
throw exceptionToThrowSupplier.get();
Review Comment:
The removed precondition check for `exceptionToThrowSupplier != null` was
valuable for defensive programming. If this supplier can be null in any code
path, a NullPointerException would be less informative than the original
precondition failure message.
##########
core/src/main/java/org/apache/gravitino/storage/relational/mapper/JobMetaMapper.java:
##########
@@ -70,4 +70,6 @@ Integer deleteJobMetasByLegacyTimeline(
@UpdateProvider(type = JobMetaSQLProviderFactory.class, method =
"softDeleteJobMetaByRunId")
Integer softDeleteJobMetaByRunId(@Param("jobRunId") Long jobRunId);
+
+ String selectJobById(Long jobId);
Review Comment:
Missing @SelectProvider annotation or implementation. This method signature
appears incomplete - similar to Comment 6, it needs an annotation or
implementation.
```suggestion
```
##########
clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/authorization/JobAuthorizationIT.java:
##########
@@ -0,0 +1,335 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.client.integration.test.authorization;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
+import java.io.File;
+import java.nio.file.Files;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.commons.io.FileUtils;
+import org.apache.gravitino.MetadataObject;
+import org.apache.gravitino.MetadataObjects;
+import org.apache.gravitino.authorization.Privileges;
+import org.apache.gravitino.client.GravitinoMetalake;
+import org.apache.gravitino.exceptions.ForbiddenException;
+import org.apache.gravitino.job.JobHandle;
+import org.apache.gravitino.job.JobTemplate;
+import org.apache.gravitino.job.ShellJobTemplate;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.MethodOrderer;
+import org.junit.jupiter.api.Order;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestMethodOrder;
+
+@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
+public class JobAuthorizationIT extends BaseRestApiAuthorizationIT {
+
+ private static File testStagingDir;
+ private static ShellJobTemplate.Builder builder;
+ private static final String ROLE = "job_test_role";
+
+ @BeforeAll
+ public void startIntegrationTest() throws Exception {
+ testStagingDir = Files.createTempDirectory("test_staging_dir").toFile();
+ String testEntryScriptPath = generateTestEntryScript();
+ String testLibScriptPath = generateTestLibScript();
+
+ builder =
+ ShellJobTemplate.builder()
+ .withComment("Test shell job template")
+ .withExecutable(testEntryScriptPath)
+ .withArguments(Lists.newArrayList("{{arg1}}", "{{arg2}}"))
+ .withEnvironments(ImmutableMap.of("ENV_VAR", "{{env_var}}"))
+ .withScripts(Lists.newArrayList(testLibScriptPath))
+ .withCustomFields(Collections.emptyMap());
+
+ Map<String, String> configs =
+ ImmutableMap.of(
+ "gravitino.job.stagingDir",
+ testStagingDir.getAbsolutePath(),
+ "gravitino.job.statusPullIntervalInMs",
+ "3000");
+ registerCustomConfigs(configs);
+ super.startIntegrationTest();
+
+ // Create role for authorization tests
+ GravitinoMetalake metalake = client.loadMetalake(METALAKE);
+ metalake.createRole(ROLE, new HashMap<>(), Collections.emptyList());
+ metalake.grantRolesToUser(ImmutableList.of(ROLE), NORMAL_USER);
+ }
+
+ @Test
+ @Order(1)
+ public void testRegisterJobTemplate() {
+ // User without privilege cannot register job template
+ assertThrows(
+ ForbiddenException.class,
+ () ->
+ normalUserClient
+ .loadMetalake(METALAKE)
+ .registerJobTemplate(builder.withName("test1").build()));
+
+ // Grant privilege to normal user
+ GravitinoMetalake metalake = client.loadMetalake(METALAKE);
+ metalake.grantPrivilegesToRole(
+ ROLE,
+ MetadataObjects.of(null, METALAKE, MetadataObject.Type.METALAKE),
+ ImmutableList.of(Privileges.RegisterJobTemplate.allow()));
+
+ // Now normal user can register job templates
+ JobTemplate template1 = builder.withName("test_1").build();
+ JobTemplate template2 = builder.withName("test_2").build();
+ normalUserClient.loadMetalake(METALAKE).registerJobTemplate(template1);
+ normalUserClient.loadMetalake(METALAKE).registerJobTemplate(template2);
+
+ // Admin can always register job templates
+ JobTemplate template3 = builder.withName("test_3").build();
+ metalake.registerJobTemplate(template3);
+ }
+
+ @Test
+ @Order(2)
+ public void testListJobTemplates() {
+ // Normal user can see job templates they own (test_1, test_2)
+ List<JobTemplate> normalUserTemplates =
+ normalUserClient.loadMetalake(METALAKE).listJobTemplates();
+ Assertions.assertEquals(2, normalUserTemplates.size());
+
+ // Admin can see all job templates (test_1, test_2, test_3)
+ List<JobTemplate> adminTemplates =
client.loadMetalake(METALAKE).listJobTemplates();
+ Assertions.assertEquals(3, adminTemplates.size());
+ }
+
+ @Test
+ @Order(3)
+ public void testGetJobTemplate() {
+ // Normal user can get their own job template
+ JobTemplate template =
normalUserClient.loadMetalake(METALAKE).getJobTemplate("test_1");
+ Assertions.assertNotNull(template);
+ Assertions.assertEquals("test_1", template.name());
+
+ // Normal user cannot get job template owned by admin
+ assertThrows(
+ ForbiddenException.class,
+ () ->
normalUserClient.loadMetalake(METALAKE).getJobTemplate("test_3"));
+
+ // Admin can get any job template
+ JobTemplate adminTemplate =
client.loadMetalake(METALAKE).getJobTemplate("test_3");
+ Assertions.assertNotNull(adminTemplate);
+ }
+
+ @Test
+ @Order(4)
+ public void testDeleteJobTemplate() {
+ // Normal user cannot delete job template owned by admin
+ assertThrows(
+ ForbiddenException.class,
+ () ->
normalUserClient.loadMetalake(METALAKE).deleteJobTemplate("test_3"));
+
+ // Normal user can delete their own job template
+ boolean deleted =
normalUserClient.loadMetalake(METALAKE).deleteJobTemplate("test_1");
+ Assertions.assertTrue(deleted);
+
+ // Admin can delete any job template
+ boolean adminDeleted =
client.loadMetalake(METALAKE).deleteJobTemplate("test_3");
+ Assertions.assertTrue(adminDeleted);
+ }
+
+ @Test
+ @Order(5)
+ public void testCreateJob() {
+ // Normal user without RunJob privilege cannot run jobs
+ assertThrows(
+ ForbiddenException.class,
+ () ->
+ normalUserClient
+ .loadMetalake(METALAKE)
+ .runJob(
+ "test_2",
+ ImmutableMap.of("arg1", "value1", "arg2", "success",
"env_var", "value2")));
+
+ // Grant RunJob privilege to normal user
+ GravitinoMetalake metalake = client.loadMetalake(METALAKE);
+ metalake.grantPrivilegesToRole(
+ ROLE,
+ MetadataObjects.of(null, METALAKE, MetadataObject.Type.METALAKE),
+ ImmutableList.of(Privileges.RunJob.allow()));
+
+ // Now normal user can run jobs on their own template
+ JobHandle normalUserJobHandle =
+ normalUserClient
+ .loadMetalake(METALAKE)
+ .runJob(
+ "test_2",
+ ImmutableMap.of("arg1", "value1", "arg2", "success",
"env_var", "value2"));
+ Assertions.assertNotNull(normalUserJobHandle);
+ Assertions.assertEquals("test_2", normalUserJobHandle.jobTemplateName());
+
+ // Admin can run jobs on any template
+ JobHandle adminJobHandle =
+ metalake.runJob(
+ "test_2", ImmutableMap.of("arg1", "value3", "arg2", "success",
"env_var", "value4"));
+ Assertions.assertNotNull(adminJobHandle);
+ }
+
+ @Test
+ @Order(6)
+ public void testListJob() {
+ // Normal user can see jobs they own (1 job from test_2 template)
+ List<JobHandle> normalUserJobs =
normalUserClient.loadMetalake(METALAKE).listJobs();
+ Assertions.assertEquals(1, normalUserJobs.size());
+
+ // Admin can see all jobs (2 jobs total)
+ List<JobHandle> adminJobs = client.loadMetalake(METALAKE).listJobs();
+ Assertions.assertEquals(2, adminJobs.size());
+
+ // Listing jobs for specific template shows all jobs from that template
+ List<JobHandle> test2Jobs =
normalUserClient.loadMetalake(METALAKE).listJobs("test_2");
+ Assertions.assertEquals(1, test2Jobs.size());
+ }
+
+ @Test
+ @Order(7)
+ public void testGetJob() {
+ // Get job IDs for testing
+ List<JobHandle> normalUserJobs =
normalUserClient.loadMetalake(METALAKE).listJobs();
+ Assertions.assertTrue(normalUserJobs.size() > 0);
+ String normalUserJobId = normalUserJobs.get(0).jobId();
+
+ List<JobHandle> adminJobs = client.loadMetalake(METALAKE).listJobs();
+ String adminJobId = null;
+ for (JobHandle job : adminJobs) {
+ if (!job.jobId().equals(normalUserJobId)) {
+ adminJobId = job.jobId();
+ break;
+ }
+ }
+ Assertions.assertNotNull(adminJobId);
+
+ // Normal user can get their own job
+ JobHandle job =
normalUserClient.loadMetalake(METALAKE).getJob(normalUserJobId);
+ Assertions.assertNotNull(job);
+ Assertions.assertEquals(normalUserJobId, job.jobId());
+
+ // Normal user cannot get job owned by admin
+ String finalAdminJobId = adminJobId;
+ assertThrows(
+ ForbiddenException.class,
+ () -> normalUserClient.loadMetalake(METALAKE).getJob(finalAdminJobId));
+
+ // Admin can get any job
+ JobHandle adminJob = client.loadMetalake(METALAKE).getJob(adminJobId);
+ Assertions.assertNotNull(adminJob);
+ }
+
+ @Test
+ @Order(8)
+ public void testCancelJob() {
+ // Get job IDs for testing
+ List<JobHandle> normalUserJobs =
normalUserClient.loadMetalake(METALAKE).listJobs();
+ Assertions.assertTrue(normalUserJobs.size() > 0);
+ String normalUserJobId = normalUserJobs.get(0).jobId();
+
+ List<JobHandle> adminJobs = client.loadMetalake(METALAKE).listJobs();
+ String adminJobId = null;
+ for (JobHandle job : adminJobs) {
+ if (!job.jobId().equals(normalUserJobId)) {
+ adminJobId = job.jobId();
+ break;
+ }
+ }
+ Assertions.assertNotNull(adminJobId);
+
+ // Normal user cannot cancel job owned by admin
+ String finalAdminJobId = adminJobId;
+ assertThrows(
+ ForbiddenException.class,
+ () ->
normalUserClient.loadMetalake(METALAKE).cancelJob(finalAdminJobId));
+
+ // Normal user can cancel their own job
+ JobHandle canceledJob =
normalUserClient.loadMetalake(METALAKE).cancelJob(normalUserJobId);
+ Assertions.assertNotNull(canceledJob);
+
+ // Admin can cancel any job
+ JobHandle adminCanceledJob =
client.loadMetalake(METALAKE).cancelJob(adminJobId);
+ Assertions.assertNotNull(adminCanceledJob);
+ }
+
+ @AfterAll
+ public static void tearDown() throws Exception {
+ if (testStagingDir != null && testStagingDir.exists()) {
+ FileUtils.deleteDirectory(testStagingDir);
+ }
+ }
+
+ private static String generateTestEntryScript() {
+ String content =
+ "#!/bin/bash\n"
+ + "echo \"starting test test job\"\n\n"
Review Comment:
Duplicate word 'test' in the echo message.
```suggestion
+ "echo \"starting test job\"\n\n"
```
##########
server/src/main/java/org/apache/gravitino/server/web/rest/JobOperations.java:
##########
@@ -258,7 +294,15 @@ public Response listJobs(
httpRequest,
() -> {
List<JobEntity> jobEntities =
- jobOperationDispatcher.listJobs(metalake,
Optional.ofNullable(jobTemplateName));
+ Lists.newArrayList(
+ MetadataAuthzHelper.filterByExpression(
+ metalake,
+ "METALAKE::OWNER || JOB::OWNER",
+ Entity.EntityType.JOB,
+ jobOperationDispatcher
+ .listJobs(metalake,
Optional.ofNullable(jobTemplateName))
+ .toArray(new JobEntity[0]),
+ jobEntity -> NameIdentifierUtil.ofJob(metalake,
jobEntity.name())));
Review Comment:
Same inefficiency as Comment 1: converting List to array and back to List.
Consider optimizing the filtering approach to avoid unnecessary conversions.
```suggestion
MetadataAuthzHelper.filterByExpression(
metalake,
"METALAKE::OWNER || JOB::OWNER",
Entity.EntityType.JOB,
jobOperationDispatcher
.listJobs(metalake,
Optional.ofNullable(jobTemplateName)),
jobEntity -> NameIdentifierUtil.ofJob(metalake,
jobEntity.name()));
```
--
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]
