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

Skip role immutable check for Roles Mappings APIs #3272

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Aug 31, 2023

Description

@willyborankin I'm opening up a Draft PR that fixes this issue locally, but wanted to get your input too. There are security-dashboards-tests failing because they cannot clean up test data by using the patch API to modify the all_access roles mapping and the request fails with this error:

> curl -XPATCH https://admin:admin@localhost:9200/_plugins/_security/api/rolesmapping/all_access --insecure -H "Content-Type: application/json" --data '
[
        {
        "op": "remove",
        "path": "/users",
        "users": ["jwt_test"]
        }
]
'
{"status":"FORBIDDEN","message":"Resource 'all_access' is static."}

I believe the fix is to skip the validateRoles check on the RolesMapping API. Essentially roles can be static or hidden, but roles mappings cannot be. Any roles mapping is editable.

Test failures seen on this PR: opensearch-project/security-dashboards-plugin#1568
Example integ test run with test failures: https://github.com/opensearch-project/security-dashboards-plugin/actions/runs/6031350472/job/16364885321?pr=1568

This failure is related to the recent change to refactor the REST APIs: #3123

  • Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Bug fix

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Merging #3272 (8da8e32) into main (0338cdd) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #3272      +/-   ##
============================================
+ Coverage     63.15%   63.18%   +0.02%     
  Complexity     3448     3448              
============================================
  Files           263      263              
  Lines         20024    20022       -2     
  Branches       3341     3341              
============================================
+ Hits          12647    12650       +3     
+ Misses         5748     5745       -3     
+ Partials       1629     1627       -2     
Files Changed Coverage Δ
.../security/dlic/rest/api/RolesMappingApiAction.java 96.87% <100.00%> (-0.19%) ⬇️

... and 1 file with indirect coverage changes

@willyborankin
Copy link
Collaborator

willyborankin commented Aug 31, 2023

@cwperks Im on it. Old code checked hidden role only. I will fix it

@cwperks
Copy link
Member Author

cwperks commented Aug 31, 2023

Closing this PR. @willyborankin is working on this and will open a PR with a proper fix shortly.

@cwperks cwperks closed this Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants