Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix-15771] Fix normal user can grant project permission #15772

Merged
merged 7 commits into from
Mar 27, 2024

Conversation

silentxingtian
Copy link
Contributor

Purpose of the pull request

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

@qingwli qingwli added first time contributor First-time contributor 3.2.2 labels Mar 27, 2024
@qingwli qingwli added this to the 3.2.2 milestone Mar 27, 2024
@qingwli qingwli linked an issue Mar 27, 2024 that may be closed by this pull request
3 tasks
@qingwli qingwli changed the title repair the bug #15771 by call the interface. [Fix-15771] Fix normal user can grant project permission vulnerability Mar 27, 2024
@qingwli
Copy link
Member

qingwli commented Mar 27, 2024

Hi @silentxingtian , how about this method?

 /**
     * grant project
     *
     * @param loginUser  login user
     * @param userId     user id
     * @param projectIds project id array
     * @return grant result code
     */
    @Override
    @Transactional
    public Map<String, Object> grantProject(User loginUser, int userId, String projectIds) {
        Map<String, Object> result = new HashMap<>();
        result.put(Constants.STATUS, false);
        if (resourcePermissionCheckService.functionDisabled()) {
            putMsg(result, Status.FUNCTION_DISABLED);
            return result;
        }
        // check exist
        User tempUser = userMapper.selectById(userId);
        if (tempUser == null) {
            log.error("User does not exist, userId:{}.", userId);
            putMsg(result, Status.USER_NOT_EXIST, userId);
            return result;
        }
        

Looks like not checking is admin too. Could you fix this?

@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 39.10%. Comparing base (5466117) to head (9b36805).

❗ Current head 9b36805 differs from pull request most recent head 2266ca2. Consider uploading reports for the commit 2266ca2 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##                dev   #15772      +/-   ##
============================================
+ Coverage     39.09%   39.10%   +0.01%     
- Complexity     4850     4853       +3     
============================================
  Files          1316     1316              
  Lines         44936    44945       +9     
  Branches       4797     4800       +3     
============================================
+ Hits          17567    17576       +9     
  Misses        25478    25478              
  Partials       1891     1891              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@davidzollo davidzollo changed the title [Fix-15771] Fix normal user can grant project permission vulnerability [Fix-15771] Fix normal user can grant project permission Mar 27, 2024
@silentxingtian
Copy link
Contributor Author

Hi @silentxingtian , how about this method?

 /**
     * grant project
     *
     * @param loginUser  login user
     * @param userId     user id
     * @param projectIds project id array
     * @return grant result code
     */
    @Override
    @Transactional
    public Map<String, Object> grantProject(User loginUser, int userId, String projectIds) {
        Map<String, Object> result = new HashMap<>();
        result.put(Constants.STATUS, false);
        if (resourcePermissionCheckService.functionDisabled()) {
            putMsg(result, Status.FUNCTION_DISABLED);
            return result;
        }
        // check exist
        User tempUser = userMapper.selectById(userId);
        if (tempUser == null) {
            log.error("User does not exist, userId:{}.", userId);
            putMsg(result, Status.USER_NOT_EXIST, userId);
            return result;
        }
        

Looks like not checking is admin too. Could you fix this?

ok, I will check all method and fix it

@SbloodyS SbloodyS added the bug Something isn't working label Mar 27, 2024
@silentxingtian
Copy link
Contributor Author

i had fix grant project ,udf ,datasource method ,please check it again

SbloodyS
SbloodyS previously approved these changes Mar 27, 2024
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM. We should hide these options in the front-end UI as improvement.

@SbloodyS
Copy link
Member

@silentxingtian Please fix UT.

@silentxingtian
Copy link
Contributor Author

normal

this method need to add admin user check ,i had fix it.

@wangxj3 wangxj3 self-requested a review March 27, 2024 12:29
@wangxj3
Copy link
Contributor

wangxj3 commented Mar 27, 2024

LGTM

@wangxj3 wangxj3 self-requested a review March 27, 2024 12:36
Copy link
Contributor

@wangxj3 wangxj3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

sonarcloud bot commented Mar 27, 2024

Copy link
Contributor

@davidzollo davidzollo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Thanks for your first contribution. I'm looking forward to your deep participation.

@davidzollo davidzollo merged commit 5e3dc7b into apache:dev Mar 27, 2024
60 checks passed
ChenYiGeEr added a commit to ChenYiGeEr/dolphinscheduler that referenced this pull request Mar 28, 2024
* apache-dolphinscheduler/dev:
  [Fix-15771] Fix normal user can grant project permission (apache#15772)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.2.2 backend bug Something isn't working first time contributor First-time contributor ready-to-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] [API] The user just can be view owner project in the default.
6 participants