From 7336afaa65e3c4bb4596081cf0049b5166506a0c Mon Sep 17 00:00:00 2001 From: rickchengx <38122586+rickchengx@users.noreply.github.com> Date: Fri, 25 Nov 2022 17:59:28 +0800 Subject: [PATCH] [Fix-12916] Add permission check when query or download log (#12917) --- .../api/controller/LoggerController.java | 4 +- .../api/service/LoggerService.java | 6 +- .../api/service/impl/LoggerServiceImpl.java | 10 ++- .../impl/ProcessInstanceServiceImpl.java | 6 +- .../api/service/LoggerServiceTest.java | 80 +++++++++++++++---- .../service/ProcessInstanceServiceTest.java | 2 +- .../dao/mapper/ProjectMapper.java | 7 ++ .../dao/mapper/ProjectMapper.xml | 12 +++ 8 files changed, 103 insertions(+), 24 deletions(-) diff --git a/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/LoggerController.java b/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/LoggerController.java index 0909d4d85c..bd2a79285d 100644 --- a/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/LoggerController.java +++ b/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/controller/LoggerController.java @@ -81,7 +81,7 @@ public class LoggerController extends BaseController { @RequestParam(value = "taskInstanceId") int taskInstanceId, @RequestParam(value = "skipLineNum") int skipNum, @RequestParam(value = "limit") int limit) { - return loggerService.queryLog(taskInstanceId, skipNum, limit); + return loggerService.queryLog(loginUser, taskInstanceId, skipNum, limit); } /** @@ -101,7 +101,7 @@ public class LoggerController extends BaseController { @AccessLogAnnotation(ignoreRequestArgs = "loginUser") public ResponseEntity downloadTaskLog(@Parameter(hidden = true) @RequestAttribute(value = Constants.SESSION_USER) User loginUser, @RequestParam(value = "taskInstanceId") int taskInstanceId) { - byte[] logBytes = loggerService.getLogBytes(taskInstanceId); + byte[] logBytes = loggerService.getLogBytes(loginUser, taskInstanceId); return ResponseEntity .ok() .header(HttpHeaders.CONTENT_DISPOSITION, diff --git a/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/LoggerService.java b/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/LoggerService.java index 33477c509e..db1cb7c465 100644 --- a/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/LoggerService.java +++ b/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/LoggerService.java @@ -31,20 +31,22 @@ public interface LoggerService { /** * view log * + * @param loginUser login user * @param taskInstId task instance id * @param skipLineNum skip line number * @param limit limit * @return log string data */ - Result queryLog(int taskInstId, int skipLineNum, int limit); + Result queryLog(User loginUser, int taskInstId, int skipLineNum, int limit); /** * get log size * + * @param loginUser login user * @param taskInstId task instance id * @return log byte array */ - byte[] getLogBytes(int taskInstId); + byte[] getLogBytes(User loginUser, int taskInstId); /** * query log diff --git a/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/LoggerServiceImpl.java b/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/LoggerServiceImpl.java index 239c9669f1..227f74c12d 100644 --- a/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/LoggerServiceImpl.java +++ b/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/LoggerServiceImpl.java @@ -77,6 +77,7 @@ public class LoggerServiceImpl extends BaseServiceImpl implements LoggerService /** * view log * + * @param loginUser login user * @param taskInstId task instance id * @param skipLineNum skip line number * @param limit limit @@ -84,7 +85,7 @@ public class LoggerServiceImpl extends BaseServiceImpl implements LoggerService */ @Override @SuppressWarnings("unchecked") - public Result queryLog(int taskInstId, int skipLineNum, int limit) { + public Result queryLog(User loginUser, int taskInstId, int skipLineNum, int limit) { TaskInstance taskInstance = taskInstanceDao.findTaskInstanceById(taskInstId); @@ -96,6 +97,8 @@ public class LoggerServiceImpl extends BaseServiceImpl implements LoggerService logger.error("Host of task instance is null, taskInstanceId:{}.", taskInstId); return Result.error(Status.TASK_INSTANCE_HOST_IS_NULL); } + Project project = projectMapper.queryProjectByTaskInstanceId(taskInstId); + projectService.checkProjectAndAuthThrowException(loginUser, project, VIEW_LOG); Result result = new Result<>(Status.SUCCESS.getCode(), Status.SUCCESS.getMsg()); String log = queryLog(taskInstance, skipLineNum, limit); int lineNum = log.split("\\r\\n").length; @@ -106,15 +109,18 @@ public class LoggerServiceImpl extends BaseServiceImpl implements LoggerService /** * get log size * + * @param loginUser login user * @param taskInstId task instance id * @return log byte array */ @Override - public byte[] getLogBytes(int taskInstId) { + public byte[] getLogBytes(User loginUser, int taskInstId) { TaskInstance taskInstance = taskInstanceDao.findTaskInstanceById(taskInstId); if (taskInstance == null || StringUtils.isBlank(taskInstance.getHost())) { throw new ServiceException("task instance is null or host is null"); } + Project project = projectMapper.queryProjectByTaskInstanceId(taskInstId); + projectService.checkProjectAndAuthThrowException(loginUser, project, DOWNLOAD_LOG); return getLogBytes(taskInstance); } diff --git a/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProcessInstanceServiceImpl.java b/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProcessInstanceServiceImpl.java index 05415f899a..4c3fceefe3 100644 --- a/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProcessInstanceServiceImpl.java +++ b/dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/service/impl/ProcessInstanceServiceImpl.java @@ -375,7 +375,7 @@ public class ProcessInstanceServiceImpl extends BaseServiceImpl implements Proce } List taskInstanceList = taskInstanceDao.findValidTaskListByProcessId(processId, processInstance.getTestFlag()); - addDependResultForTaskList(taskInstanceList); + addDependResultForTaskList(loginUser, taskInstanceList); Map resultMap = new HashMap<>(); resultMap.put(PROCESS_INSTANCE_STATE, processInstance.getState().toString()); resultMap.put(TASK_LIST, taskInstanceList); @@ -388,12 +388,12 @@ public class ProcessInstanceServiceImpl extends BaseServiceImpl implements Proce /** * add dependent result for dependent task */ - private void addDependResultForTaskList(List taskInstanceList) throws IOException { + private void addDependResultForTaskList(User loginUser, List taskInstanceList) throws IOException { for (TaskInstance taskInstance : taskInstanceList) { if (TASK_TYPE_DEPENDENT.equalsIgnoreCase(taskInstance.getTaskType())) { logger.info("DEPENDENT type task instance need to set dependent result, taskCode:{}, taskInstanceId:{}", taskInstance.getTaskCode(), taskInstance.getId()); - Result logResult = loggerService.queryLog( + Result logResult = loggerService.queryLog(loginUser, taskInstance.getId(), Constants.LOG_QUERY_SKIP_LINE_NUMBER, Constants.LOG_QUERY_LIMIT); if (logResult.getCode() == Status.SUCCESS.ordinal()) { String log = logResult.getData().getMessage(); diff --git a/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/LoggerServiceTest.java b/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/LoggerServiceTest.java index ca34d74730..000a5955a4 100644 --- a/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/LoggerServiceTest.java +++ b/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/LoggerServiceTest.java @@ -21,6 +21,7 @@ import static org.apache.dolphinscheduler.api.constants.ApiFuncIdentificationCon import static org.apache.dolphinscheduler.api.constants.ApiFuncIdentificationConstant.VIEW_LOG; import org.apache.dolphinscheduler.api.enums.Status; +import org.apache.dolphinscheduler.api.exceptions.ServiceException; import org.apache.dolphinscheduler.api.service.impl.LoggerServiceImpl; import org.apache.dolphinscheduler.api.utils.Result; import org.apache.dolphinscheduler.common.constants.Constants; @@ -78,60 +79,111 @@ public class LoggerServiceTest { private LogClient logClient; @Test - public void testQueryDataSourceList() { + public void testQueryLog() { + User loginUser = new User(); + loginUser.setId(1); TaskInstance taskInstance = new TaskInstance(); + taskInstance.setExecutorId(loginUser.getId() + 1); Mockito.when(taskInstanceDao.findTaskInstanceById(1)).thenReturn(taskInstance); - Result result = loggerService.queryLog(2, 1, 1); + Result result = loggerService.queryLog(loginUser, 2, 1, 1); // TASK_INSTANCE_NOT_FOUND Assertions.assertEquals(Status.TASK_INSTANCE_NOT_FOUND.getCode(), result.getCode().intValue()); try { // HOST NOT FOUND OR ILLEGAL - result = loggerService.queryLog(1, 1, 1); + result = loggerService.queryLog(loginUser, 1, 1, 1); } catch (RuntimeException e) { Assertions.assertTrue(true); logger.error("testQueryDataSourceList error {}", e.getMessage()); } Assertions.assertEquals(Status.TASK_INSTANCE_HOST_IS_NULL.getCode(), result.getCode().intValue()); - // SUCCESS + // PROJECT_NOT_EXIST taskInstance.setHost("127.0.0.1:8080"); taskInstance.setLogPath("/temp/log"); + try { + Mockito.doThrow(new ServiceException(Status.PROJECT_NOT_EXIST)).when(projectService) + .checkProjectAndAuthThrowException(loginUser, null, VIEW_LOG); + loggerService.queryLog(loginUser, 1, 1, 1); + } catch (ServiceException serviceException) { + Assertions.assertEquals(Status.PROJECT_NOT_EXIST.getCode(), serviceException.getCode()); + } + + // USER_NO_OPERATION_PERM + Project project = getProject(1); + Mockito.when(projectMapper.queryProjectByTaskInstanceId(1)).thenReturn(project); + try { + Mockito.doThrow(new ServiceException(Status.USER_NO_OPERATION_PERM)).when(projectService) + .checkProjectAndAuthThrowException(loginUser, project, VIEW_LOG); + loggerService.queryLog(loginUser, 1, 1, 1); + } catch (ServiceException serviceException) { + Assertions.assertEquals(Status.USER_NO_OPERATION_PERM.getCode(), serviceException.getCode()); + } + + // SUCCESS + Mockito.doNothing().when(projectService).checkProjectAndAuthThrowException(loginUser, project, VIEW_LOG); Mockito.when(taskInstanceDao.findTaskInstanceById(1)).thenReturn(taskInstance); - result = loggerService.queryLog(1, 1, 1); + result = loggerService.queryLog(loginUser, 1, 1, 1); Assertions.assertEquals(Status.SUCCESS.getCode(), result.getCode().intValue()); } @Test public void testGetLogBytes() { + User loginUser = new User(); + loginUser.setId(1); TaskInstance taskInstance = new TaskInstance(); + taskInstance.setExecutorId(loginUser.getId() + 1); Mockito.when(taskInstanceDao.findTaskInstanceById(1)).thenReturn(taskInstance); // task instance is null try { - loggerService.getLogBytes(2); - } catch (RuntimeException e) { - Assertions.assertTrue(true); + loggerService.getLogBytes(loginUser, 2); + } catch (ServiceException e) { + Assertions.assertEquals(new ServiceException("task instance is null or host is null").getMessage(), + e.getMessage()); logger.error("testGetLogBytes error: {}", "task instance is null"); } // task instance host is null try { - loggerService.getLogBytes(1); - } catch (RuntimeException e) { - Assertions.assertTrue(true); + loggerService.getLogBytes(loginUser, 1); + } catch (ServiceException e) { + Assertions.assertEquals(new ServiceException("task instance is null or host is null").getMessage(), + e.getMessage()); logger.error("testGetLogBytes error: {}", "task instance host is null"); } - // success + // PROJECT_NOT_EXIST taskInstance.setHost("127.0.0.1:8080"); taskInstance.setLogPath("/temp/log"); + try { + Mockito.doThrow(new ServiceException(Status.PROJECT_NOT_EXIST)).when(projectService) + .checkProjectAndAuthThrowException(loginUser, null, DOWNLOAD_LOG); + loggerService.queryLog(loginUser, 1, 1, 1); + } catch (ServiceException serviceException) { + Assertions.assertEquals(Status.PROJECT_NOT_EXIST.getCode(), serviceException.getCode()); + } + + // USER_NO_OPERATION_PERM + Project project = getProject(1); + Mockito.when(projectMapper.queryProjectByTaskInstanceId(1)).thenReturn(project); + try { + Mockito.doThrow(new ServiceException(Status.USER_NO_OPERATION_PERM)).when(projectService) + .checkProjectAndAuthThrowException(loginUser, project, DOWNLOAD_LOG); + loggerService.queryLog(loginUser, 1, 1, 1); + } catch (ServiceException serviceException) { + Assertions.assertEquals(Status.USER_NO_OPERATION_PERM.getCode(), serviceException.getCode()); + } + + // SUCCESS + Mockito.doNothing().when(projectService).checkProjectAndAuthThrowException(loginUser, project, DOWNLOAD_LOG); Mockito.when(logClient.getLogBytes(Mockito.anyString(), Mockito.anyInt(), Mockito.anyString())) .thenReturn(new byte[0]); - loggerService.getLogBytes(1); - + Mockito.when(projectMapper.queryProjectByTaskInstanceId(1)).thenReturn(project); + byte[] result = loggerService.getLogBytes(loginUser, 1); + Assertions.assertEquals(90, result.length); } @Test diff --git a/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/ProcessInstanceServiceTest.java b/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/ProcessInstanceServiceTest.java index f6bbefd8e4..abf29bbc5b 100644 --- a/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/ProcessInstanceServiceTest.java +++ b/dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/ProcessInstanceServiceTest.java @@ -414,7 +414,7 @@ public class ProcessInstanceServiceTest { .thenReturn(Optional.of(processInstance)); when(taskInstanceDao.findValidTaskListByProcessId(processInstance.getId(), processInstance.getTestFlag())) .thenReturn(taskInstanceList); - when(loggerService.queryLog(taskInstance.getId(), 0, 4098)).thenReturn(res); + when(loggerService.queryLog(loginUser, taskInstance.getId(), 0, 4098)).thenReturn(res); Map successRes = processInstanceService.queryTaskListByProcessId(loginUser, projectCode, 1); Assertions.assertEquals(Status.SUCCESS, successRes.get(Constants.STATUS)); } diff --git a/dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/mapper/ProjectMapper.java b/dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/mapper/ProjectMapper.java index 59c635f711..c43a2415f0 100644 --- a/dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/mapper/ProjectMapper.java +++ b/dolphinscheduler-dao/src/main/java/org/apache/dolphinscheduler/dao/mapper/ProjectMapper.java @@ -143,4 +143,11 @@ public interface ProjectMapper extends BaseMapper { * @return projectList */ List queryAllProjectForDependent(); + + /** + * query the project by task instance id + * @param taskInstanceId + * @return project + */ + Project queryProjectByTaskInstanceId(@Param("taskInstanceId") int taskInstanceId); } diff --git a/dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/ProjectMapper.xml b/dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/ProjectMapper.xml index d01d970ee1..6996d6f754 100644 --- a/dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/ProjectMapper.xml +++ b/dolphinscheduler-dao/src/main/resources/org/apache/dolphinscheduler/dao/mapper/ProjectMapper.xml @@ -185,4 +185,16 @@ select code, name from t_ds_project + +