[Resource Access Control] [Part2] Updates SPI to introduce a ResourceSharingClient interface and implements it within security plugin

[DarshitChanpura]

Continuation of #5194 with a no-client approach.

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

Description

Modifies SPI to add a client to invoke resource-access-control APIs.

There are 4 java APIs as well as 4 REST APIs introduced as part of this PR. Plugins will leverage the client to call the java APIs to implement resource access control.

Refer to the [RESOURCE_ACCESS_CONTROL_FOR_PLUGINS.md](https://github.com/DarshitChanpura/security/blob/resource-sharing-no-client/RESOURCE_ACCESS_CONTROL_FOR_PLUGINS.md file to understand in-depth implementation of this feature.

Issues Resolved

• Resolves [Proposal] Resource Permissions and Sharing #4500

Testing

• automated tests + manual 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]

@nibix @cwperks @derek-ho The SPI is now required to be declared as compileOnly instead of implementation. This was done by removing the requirement for ResourceParser and converting listAccessibleResources API from returning actual resources to just resourceIds.

[cwperks]

Are these steps required?

[DarshitChanpura]

Yes, CI checks will fail without these.

[cwperks]

For plugins that implement this extension, can't we rely on the the ResourceSharingClient never being provided as having either 1) security uninstalled, 2) security disabled or 3) this feature disabled?

I see that there are checks added in the AD PR to see if security is disabled, but I don't think the checks should be present in those repos.

[DarshitChanpura]

I see that there are checks added in the AD PR to see if security is disabled, but I don't think the checks should be present in those repos.

The checks are meant to be for the feature to be disabled. If you do see a security enabled/disabled check then it is a miss on my end. Would you mind pointing me to the exact code change?

[cwperks]

This should remain package-private

[cwperks]

wdyt about only having this setter and then computing everything else within the ResourcePluginInfo class?

[DarshitChanpura]

We have an opportunity to preload the information that will not change, and will allows us to have constant time look-ups instead of computing these from resourceSharingExtensions. I think we should keep it as is.

[cwperks]

Is this wrapper class needed? Looks like you can call this.resourceSharingIndexHandler.createResourceSharingIndexIfAbsent directly down below. What is the purpose of the callable as an arg to that method?

[DarshitChanpura]

I've followed suit with existing pattern:

security/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java

Line 320 in 7603dc2

private boolean createSecurityIndexIfAbsent() {

This will allows us to separate any index management tasks to its own class. Callable was a placeholder. I have removed extra abstraction since the only method offer by the repository was to create the index. I've replaced it with direct call

[cwperks]

Thank you @DarshitChanpura, I think most large structural items have been addressed. I think we should keep references to the feature flag only in this repo and not expose those outside the repo.

I am having trouble reviewing the queries in the ResourceSharingIndexHandler where its computing the resources visible to the current user, but the code and the intent is easy to follow.

I think this PR is a big step forward towards sensible authz for resources created by plugins where this lays the groundwork towards removing the copies of the users stored across various plugin system indices. Even with the placeholder for action groups at this point, these set of PRs can be built upon towards the desired end state of resource authorization where the owner of the resource gets to specify the access level when sharing.

While this change is large, it is done in a way not to cause disruption to the certain system and is enabled by a new feature flag. This is particularly good for anyone implementing a plugin from scratch to have a mechanism they can leverage from the security plugin and also allows for an opt-in model where existing plugins can opt into this mechanism from the security plugin to avoid a lot of existing code duplication around the plugin ecosystem.

I'm very eager to see the next steps after this initial groundwork to truly move towards the new model of resource authorization. This is a benefit for 1) OpenSearch users that will be empowered to control how their resources are shared, 2) plugin developers which can now delegate resource authz to the security plugin and 3) cluster administrators will not be bottlenecked figuring out how to do resource sharing using the existing filter_by_backend_role mechanism.

This PR is against a feature branch and a separate PR will be done to the main branch.

[cwperks]

My highest-level comments have been addressed. Have you been able to organize it in such a way where only this repo refers to the plugins.security.resource_sharing.enabled feature flag?

Marking PR approved to merge into the feature branch, another review will be performed before merging to main.

[DarshitChanpura]

Have you been able to organize it in such a way where only this repo refers to the plugins.security.resource_sharing.enabled feature flag?

I see two potential ways to handle plugin's backwards compatibility atm:

1. Make this feature flag available to plugins to use it to control new vs present implementation:
   a. Checkout this example in AD plugin PR which controls the path for feature enabled vs disabled. https://github.com/opensearch-project/anomaly-detection/pull/1400/files#diff-cf370d40fdd2abc404283fa1e37768646f30ee1b8bfbb0b5c5302fc8a2a080fcR707

2. Instead of relying on feature flag directly, they now check whether client is instanceOf NoopResourceSharingClient to process the route forward.

Feature-flag made sense to me in terms of readability compared to option 2. Let me know your thoughts.

[DarshitChanpura]

Merging this. All future comments can be addressed in PR towards main.