New algorithm for resolving action groups

[nibix]

Description

This code change is just in preparation for the change in #4380 as requested in #4416 (review) .

This replaces the previous action group resolution algorithm done in ConfigModelV7 by a new one, which has a number of enhancements:

• The new implementation forms an independent class, enhancing testability and use in other components that will be introduced in Optimized Privilege Evaluation #4380.
• The new implementation handles action group configurations containing loops gracefully. The old one would wind up StackOverflowErrors.
• The new one pre-computes the resolved action groups, thus enhancing the performance.

• Category: Enhancement
• Why these changes are required? - The changes in Optimized Privilege Evaluation #4380 will no longer use ConfigModelV7 and thus need an independent action group resolution mechanism.
• What is the old behavior before changes and new behavior after changes? - No behavioral changes except for action group definitions containing groups, which will be handled gracefully now.

Testing

• Unit tests in org.opensearch.security.securityconf.FlattenedActionGroupsTest

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]

Codecov Report

Attention: Patch coverage is 82.75862% with 10 lines in your changes missing coverage. Please review.

Project coverage is 65.48%. Comparing base (94f7ccb) to head (110426f).
Report is 5 commits behind head on main.

❗ Current head 110426f differs from pull request most recent head 289e9dd

Please upload reports for the commit 289e9dd to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4448      +/-   ##
==========================================
+ Coverage   65.45%   65.48%   +0.03%     
==========================================
  Files         312      313       +1     
  Lines       22042    22064      +22     
  Branches     3559     3562       +3     
==========================================
+ Hits        14427    14449      +22     
+ Misses       5843     5840       -3     
- Partials     1772     1775       +3     

Files	Coverage Δ
...ecurityconf/impl/SecurityDynamicConfiguration.java	80.30% <100.00%> (+0.77%)	⬆️
...pensearch/security/securityconf/ConfigModelV7.java	67.74% <80.00%> (-0.18%)	⬇️
...h/security/securityconf/FlattenedActionGroups.java	81.25% <81.25%> (ø)

... and 3 files with indirect coverage changes

[shikharj05]

LGTM 👍

[cwperks]

Thank you @nibix! This change looks good to me. One scenario I'd like to see covered would be a change in action group and verify its reflected in the new FlattenedActionGroups data structure.

Would it make sense to included the flattened actions in the API response for GET _plugins/_security/api/actiongroups/<action-group>?

https://opensearch.org/docs/latest/security/access-control/api/#get-action-group

[DarshitChanpura]

@nibix the changes look good to me. Would the new runtime be more linear compared to the older implementation because we are using a combination of BFS + DFS?

[nibix]

@cwperks

Thank you @nibix! This change looks good to me. One scenario I'd like to see covered would be a change in action group and verify its reflected in the new FlattenedActionGroups data structure.

This is certainly an important thing to test. However, such configuration updates are handled by the DynamicConfigFactory class which is not changed by this PR.

There used to be an integration test which would test what you are asking for - that is, it changed action groups via API and tested the effectiveness of these changes via normal user operations. That was at https://github.com/opensearch-project/security/blob/069249656970e839302a0d3b8b87c82fbf3e56cc/src/test/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiTest.java . There was recently a refactor of this test in #4371. I quickly skimmed the replacement code and could not find any logic which tests the effectiveness of the modified configuration, but maybe I missed something?

[nibix]

@DarshitChanpura

Would the new runtime be more linear compared to the older implementation because we are using a combination of BFS + DFS?

The major gain comes from the fact that we calculate the resolved action group space only once instead of re-calculating it again and again for each action privilege configuration. Additionally, by using an iterative algorithm instead of a recursive algorithm it is easier for us to detect that resolution has finished and abort early. That give some more gain. However, all these gains only pertain to the config reload phase and not to the actual privilege evaluation.

[DarshitChanpura]

Merging this since we have approvals. Any discussions can be addressed in follow-up PRs.