security icon indicating copy to clipboard operation
security copied to clipboard

Fixed use of rolesMappingConfiguration in InternalUsersApiActionValidationTest

Open nibix opened this issue 1 year ago • 4 comments

Description

While working on https://github.com/opensearch-project/security/pull/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

  • [x] 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.

nibix avatar Sep 21 '24 18:09 nibix

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

codecov[bot] avatar Sep 21 '24 18:09 codecov[bot]

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.

cwperks avatar Sep 23 '24 16:09 cwperks

@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!

nibix avatar Sep 24 '24 04:09 nibix

@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 avatar Sep 24 '24 04:09 nibix