[Fix-12916] Add permission check when query or download log

[rickchengx]

Purpose of the pull request

• close: [Bug] [Log] User can query or download other user's log #12916

Currently, there is no permission check when user query or download log by task instance id.

Brief change log

Add permission check when query or download log

Who can query or download log ?

All users who have the permission of the project to which the task instance belongs can query or download the log.

1. Query the project to which the task instance belongs from the database.

2. Check the permission of project by projectService.checkProjectAndAuth()

Verify this pull request

covered by UT and manually tested as below

Note that the task instance id 1 is not created by current login user.

[codecov-commenter]

Codecov Report

Merging #12917 (54014d2) into dev (38b8767) will increase coverage by 0.01%.
The diff coverage is 62.50%.

@@             Coverage Diff              @@
##                dev   #12917      +/-   ##
============================================
+ Coverage     39.35%   39.36%   +0.01%     
  Complexity     4271     4271              
============================================
  Files          1067     1067              
  Lines         40121    40125       +4     
  Branches       4601     4605       +4     
============================================
+ Hits          15791    15797       +6     
  Misses        22551    22551              
+ Partials       1779     1777       -2     

Impacted Files	Coverage Δ
...phinscheduler/api/controller/LoggerController.java	6.66% <0.00%> (ø)
...r/api/service/impl/ProcessInstanceServiceImpl.java	70.37% <50.00%> (ø)
...nscheduler/api/service/impl/LoggerServiceImpl.java	72.30% <100.00%> (+1.81%)	⬆️
...e/dolphinscheduler/remote/NettyRemotingClient.java	52.77% <0.00%> (+0.69%)	⬆️
...er/master/dispatch/host/assign/RandomSelector.java	83.33% <0.00%> (+5.55%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[SbloodyS]

I dont't think it's a bug. It need to be disscussed. cc @ruanwenjun @caishunfeng @EricGao888 @zhongjiajie

[rickchengx]

I dont't think it's a bug. It need to be disscussed. cc @ruanwenjun @caishunfeng @EricGao888 @zhongjiajie

Hi, @SbloodyS , thanks for your comment.

Here is an example to illustrate why I think this is a bug:

• User 1 has a project project-1, and an task-instance-1 (suppose taskInstanceId=1)
• User 2 has no permission on project-1 , and he cannot see the project-1 and the task-instance-1 on the UI. But he can easily query the log of task-instance-1 by sending a GET http /dolphinscheduler/log/detail?taskInstanceId=1&skipLineNum=0&limit=1000. He only needs to set an taskInstanceId, which is not randomly generated.

In more serious cases, the logs may contain sensitive information

• E.g., the log of Sqoop task will output the mysql password (BTW, this problem will be fixed in [Feature-10498] Mask the password in the log of sqoop task #11589 )
  Then User 2 will get the sensitive information to which he does not have permission.

[Radeity]

I dont't think it's a bug. It need to be disscussed. cc @ruanwenjun @caishunfeng @EricGao888 @zhongjiajie

I agree with @rickchengx, users may make bad log decisions which will lead to security issue, so there's a strong need to for this permission check. In addition, i suggest to add request to apply for temporary permission to query and download logs for users who don't have permission, and this application can be approved by administrator.

[rickchengx]

I dont't think it's a bug. It need to be disscussed. cc @ruanwenjun @caishunfeng @EricGao888 @zhongjiajie

I agree with @rickchengx, users may make bad log decisions which will lead to security issue, so there's a strong need to for this permission check. In addition, i suggest to add request to apply for temporary permission to query and download logs for users who don't have permission, and this application can be approved by administrator.

Hi, @Radeity Thanks for your comment.

I agree that permission check is required when user query or download task logs.

Can admin authorize the log permission to the user in this PR?

Currently, admin can authorize users for the following resources: (E.g., project / resource / ....)

And in the design of this PR:
Only users with project permissions to which the task instance belongs can query or download the logs of that task instance.

Because I think the log of the task can be viewed only if the user can view the task.

So in this PR, admin can authorize the log permission to the user by authorize users for the project resource.

As for what you said, whether the administrator can individually add the log permission of a certain task instance to the user, I think it can be discussed and done after solving this security problem.

WDYT

cc @SbloodyS @EricGao888 @ruanwenjun @caishunfeng @zhongjiajie

[zhongjiajie]

I dont't think it's a bug. It need to be disscussed. cc @ruanwenjun @caishunfeng @EricGao888 @zhongjiajie

I agree with @rickchengx, users may make bad log decisions which will lead to security issue, so there's a strong need to for this permission check. In addition, i suggest to add request to apply for temporary permission to query and download logs for users who don't have permission, and this application can be approved by administrator.

Hi, @Radeity Thanks for your comment.

I agree that permission check is required when user query or download task logs.

Can admin authorize the log permission to the user in this PR?

Currently, admin can authorize users for the following resources: (E.g., project / resource / ....)

And in the design of this PR: Only users with project permissions to which the task instance belongs can query or download the logs of that task instance.

Because I think the log of the task can be viewed only if the user can view the task.

So in this PR, admin can authorize the log permission to the user by authorize users for the project resource.

As for what you said, whether the administrator can individually add the log permission of a certain task instance to the user, I think it can be discussed and done after solving this security problem.

WDYT

cc @SbloodyS @EricGao888 @ruanwenjun @caishunfeng @zhongjiajie

I think we keep permission within the project level is acceptable. we already supported it and it is simple enough

[zhongjiajie]

Ohters LGTM

[zhongjiajie]

could you use checkProjectAndAuthThrowException instead of checkProjectAndAuth to make our code more clear? It is may ask to change our UT as well

[rickchengx]

Hi, @zhongjiajie Thanks for your suggestion. I've changed to checkProjectAndAuthThrowException.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

62.5% Coverage
0.0% Duplication

[EricGao888]

PTAL when available, if no more comments, will proceed to merge.