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

Fixed use of rolesMappingConfiguration in InternalUsersApiActionValidationTest #4744

Merged

Conversation

nibix
Copy link
Collaborator

@nibix nibix commented Sep 21, 2024

Description

While working on #4546, I encountered weird behavior in InternalUsersApiActionValidationTest. Closer analysis revealed that the behavior came from the fact that the test erroneously parsed a roles mapping JSON config as CType.ROLES and not as CType.ROLESMAPPING.

I have now switched the config type to CType.ROLESMAPPING.

This has a couple of consequences:

  • The roles mapping config type does not have a static property, so it cannot be marked as such.
  • This causes the behavior of one test to flip: InternalUsersApiActionValidationTest.validateSecurityRolesWithImmutableRolesMappingConfig, last assert.

Side note: To be honest, I am a bit confused by the test validateSecurityRolesWithImmutableRolesMappingConfig - why should have a user no reserved roles or static roles assigned? Despite the change, the test demonstrates that reserved roles cannot be assigned.

  • Category: Test fix
  • Why these changes are required? - As the changes in !4546 do some more strict validations, the wrong assignment of the wrong config type leads to issues.
  • What is the old behavior before changes and new behavior after changes? - as this is a test fix, no behavior changes.

Check List

  • 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.

…ationTest

Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
Copy link

codecov bot commented Sep 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.54%. Comparing base (a1c0c7b) to head (a26c096).
Report is 20 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4744      +/-   ##
==========================================
+ Coverage   65.22%   65.54%   +0.32%     
==========================================
  Files         318      319       +1     
  Lines       22327    22470     +143     
  Branches     3591     3604      +13     
==========================================
+ Hits        14562    14728     +166     
+ Misses       5967     5932      -35     
- Partials     1798     1810      +12     

see 23 files with indirect coverage changes

@cwperks
Copy link
Member

cwperks commented Sep 23, 2024

My understanding is that reserved roles can be assigned, but that the test is pertaining to roles_mappings. There is one instance in the demo configuration of a reserved role mapping and that is kibana_server. The test is asserting that the reserved roles_mapping cannot be modified.

If its not possible to create a static roles mapping, then I think the test scenario can be removed.

@nibix
Copy link
Collaborator Author

nibix commented Sep 24, 2024

@cwperks

If its not possible to create a static roles mapping, then I think the test scenario can be removed.

Good point, removing the part!

… is no such functionality

Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
@nibix
Copy link
Collaborator Author

nibix commented Sep 24, 2024

@cwperks

My understanding is that reserved roles can be assigned, but that the test is pertaining to roles_mappings

Yes, it operate mostly on role mapping docs, but it is part of the internal users API. That's what confuses me a bit.

@nibix nibix mentioned this pull request Sep 24, 2024
1 task
@willyborankin willyborankin merged commit a62e99a into opensearch-project:main Sep 24, 2024
42 checks passed
@willyborankin willyborankin added the backport 2.x backport to 2.x branch label Sep 24, 2024
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 24, 2024
…ationTest (#4744)

Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
(cherry picked from commit a62e99a)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants