Cleaned up use of PrivilegesEvaluatorResponse

[nibix]

Description

This PR is in preparation of #5399

The class PrivilegesEvaluatorResponse was using some less-than-optimal implementation patterns, especially exposing public attribute members which were directly manipulated by other classes. Additionally, the isComplete() pattern was never really used in a meaningful way.

This PR does the following changes:

• All member attributes of PrivilegeEvaluatorResponse are private
• Instances of the class PrivilegeEvaluatorResponse are now immutable
• The isComplete() pattern is given up. Instead, methods now return a non-null PrivilegesEvaluatorResponse if they can decide on the authorization status and null otherwise.
• Some unused code is removed

• Category: Refactoring
• Why these changes are required? Preparation for Introduced explicit index resolution API #5399
• What is the old behavior before changes and new behavior after changes? No changes

Testing

• existing tests

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.

[codecov]

Codecov Report

❌ Patch coverage is 96.70330% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.49%. Comparing base (5212eed) to head (44e5a41).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...rity/privileges/ProtectedIndexAccessEvaluator.java	75.00%	1 Missing ⚠️
...ecurity/privileges/SystemIndexAccessEvaluator.java	92.30%	1 Missing ⚠️
...security/privileges/TermsAggregationEvaluator.java	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5804      +/-   ##
==========================================
- Coverage   73.52%   73.49%   -0.03%     
==========================================
  Files         438      438              
  Lines       26746    26709      -37     
  Branches     3960     3959       -1     
==========================================
- Hits        19664    19629      -35     
- Misses       5197     5200       +3     
+ Partials     1885     1880       -5     

Files with missing lines	Coverage Δ
...org/opensearch/security/filter/SecurityFilter.java	66.52% <100.00%> (+0.55%)	⬆️
...opensearch/security/filter/SecurityRestFilter.java	88.19% <100.00%> (+1.00%)	⬆️
...ch/security/privileges/PitPrivilegesEvaluator.java	96.00% <100.00%> (-0.16%)	⬇️
...h/security/privileges/PrivilegesEvaluatorImpl.java	82.85% <100.00%> (-0.25%)	⬇️
...curity/privileges/PrivilegesEvaluatorResponse.java	94.11% <100.00%> (+5.65%)	⬆️
...h/security/privileges/ResourceAccessEvaluator.java	60.71% <100.00%> (-3.81%)	⬇️
...urity/privileges/RestLayerPrivilegesEvaluator.java	93.75% <100.00%> (ø)
.../security/privileges/SnapshotRestoreEvaluator.java	96.55% <100.00%> (-0.51%)	⬇️
...rity/privileges/ProtectedIndexAccessEvaluator.java	75.55% <75.00%> (+1.08%)	⬆️
...ecurity/privileges/SystemIndexAccessEvaluator.java	76.64% <92.30%> (-0.99%)	⬇️
... and 1 more

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[DarshitChanpura]

Thank you for adding this improvement @nibix .

i do see that there is mixed usage of .reason("...") method chained with .insufficient(action). Should we have all insufficient() chained with reason()?

[nibix]

i do see that there is mixed usage of .reason("...") method chained with .insufficient(action). Should we have all insufficient() chained with reason()?

I would recommend to improve incrementally. There's already a completely new PrivilegesEvaluator implementation in #5399 ... this PR has the goal to incrementally move towards that.