[Resource Access Control] [Part3] Introduces a sample plugin to demonstrate usage of Resource Access Control feature

[DarshitChanpura]

#5016 is being broken down into smaller pieces. This is part 3.

Description

Introduces a sample resource plugin that demonstrate usage of opensearch-security-spi for resource access control.

Issues Resolved

Related to #4500

Testing

• automated tests

Check List

• New functionality includes testing
• New functionality has been documented
  ~- [ ] New Roles/Permissions have a corresponding security dashboards plugin PR
  - [ ] API changes companion pull request created
• 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.

[DarshitChanpura]

CI should pass once #5185 and #5186 are merged.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 28 lines in your changes missing coverage. Please review.

Project coverage is 70.52%. Comparing base (adf96f1) to head (83483c7).
Report is 1 commits behind head on feature/resource-permissions.

Files with missing lines	Patch %	Lines
...ecurity/resources/ResourceSharingIndexHandler.java	0.00%	15 Missing ⚠️
...arch/security/resources/ResourceAccessHandler.java	0.00%	13 Missing ⚠️

Additional details and impacted files

@@                        Coverage Diff                        @@
##           feature/resource-permissions    #5187       +/-   ##
=================================================================
+ Coverage                              0   70.52%   +70.52%     
=================================================================
  Files                                 0      342      +342     
  Lines                                 0    23175    +23175     
  Branches                              0     3591     +3591     
=================================================================
+ Hits                                  0    16344    +16344     
- Misses                                0     5057     +5057     
- Partials                              0     1774     +1774     

Files with missing lines	Coverage Δ
...g/opensearch/security/dlic/rest/support/Utils.java	58.66% <ø> (ø)
...arch/security/resources/ResourceAccessHandler.java	4.16% <0.00%> (ø)
...ecurity/resources/ResourceSharingIndexHandler.java	2.77% <0.00%> (ø)

... and 339 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.

[nibix]

I'd recommend to avoid super classes for test classes, as these take away flexibility and composability, as you can have only one super class. Additionally, the inherited @test methods make it quite difficult to understand what is being tested. IMHO, one should follow the "composition over inheritance" pattern and use references to other classes intead.

[DarshitChanpura]

The tests are common for feature enabled scenarios. This class is inherited by 3 tests which implement 3 scenarios: User with full access, user with limited access and direct access when system index feature is disabled. The super-class test suite was added only to avoid duplicated tests-code.

[nibix]

Using @SuppressWarnings("unchecked") on untrusted input (the request body source) bears the risk of getting ClassCastExceptions deep inside the application code. While this is usually not security critical, it might locating the root cause of issues diificult - both for admins and for end users.

[DarshitChanpura]

I agree. However this is is a sample plugin and hence should be fine IMO.

[nibix]

Well, the primary purpse of sample plugins is to give other developers an idea/blueprint on how to design their own code. Thus, there is a big chance that developers will follow any pattern displayed in that code base, including bad patterns.

And, experience shows that developers have the tendency to follow patterns they are finding in existing code bases, no matter how bad these actually might be.

IMHO, to cope for this, we should add in such cases at least a comment like "this is just done for simplicity, do not do that for real plugins".

[DarshitChanpura]

We do use this pattern at a decent number of places in security plugin. Here is one such example. I will add a comment here stating suppressing should be avoided in real-world, however there is no mechanism to stop plugin's from implementing it.

[nibix]

There are different "risk classes" of using @SuppressWarnings("unchecked"). For the example you have linked, it is possible to argue within a minute that this is actually a safe cast and no late ClassCastExceptions can be caused by it.

[DarshitChanpura]

Merged this feature-branch PR as i didn't see amy blocking reviews. The PR against main is already open and is the last step to getting this feature merged.