Onboards to centralized resource access control for detectors and forecasters #1533

DarshitChanpura · 2025-07-29T18:40:12Z

Description

This PR refactors changes introduced in #1400 to utilize auto-evaluation offered by security plugin.
This skips reliance on AD plugin to call verifyAccess

NOTE: Pipelines will fail until opensearch-project/security#5408 is merged.

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

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 · 2025-07-29T18:42:20Z

src/main/java/org/opensearch/timeseries/rest/handler/AbstractTimeSeriesActionHandler.java

-                    Map<Recipient, Set<String>> recipients = Map.of(Recipient.BACKEND_ROLES, Set.copyOf(user.getBackendRoles()));
-                    ShareWith shareWith = new ShareWith(Map.of(PLACE_HOLDER, new Recipients(recipients)));
-
-                    resourceSharingClient.share(configId, configIndex, shareWith, ActionListener.wrap(resourceSharing -> {


we will no longer automatically share with backend roles upon config creation. Instead, users will now have to explicitly share the config with entities at certain access-levels. This will be done via explicit REST api calls or via dashboard component. Share API is being introduced here: opensearch-project/security#5459

Few questions:

Does this mean backend role filtering enabled setting doesn't do anything?

If this is enabled we now no longer save check the backend role when doing operations?

After creating detector/config they will need to call the share api if they want anyone but themselves to have access?

will admin still have access to all configs/detectors

After talking more offline, this is my current understanding:

We are still gonna have some support of our backend role filtering but this new authz will be used if both flags are enabled, see: Onboards to centralized resource access control for detectors and forecasters #1533 (comment)

we have the backend role as part of the user information but that isn't what dictates access to the detector, the access to the resource will be dictated based on permission granted in action group as part of security plugin

yes for example: after creating a detector even if I have my own backend role of "HR" this isn't applied to the detector, if I want everyone with that backend role to be able to view the detector I need to call on share api to add backend role to the config ID for example for READ_ONLY or READ_WRITE

Only super admin will have access, just getting * (all access admin) isn't enough to see all resources that are not shared.

DarshitChanpura · 2025-07-29T18:43:56Z

src/main/java/org/opensearch/timeseries/transport/JobRequest.java

 import org.opensearch.timeseries.model.DateRange;

-public class JobRequest extends ActionRequest {
+public class JobRequest extends ActionRequest implements DocRequest {


this is how resource access requests are filtered for evaluation. Check out implementation here: https://github.com/opensearch-project/security/pull/5408/files#diff-5892c7b9dfd08da869c293e469b09654309be9994205ca939f0ce9e336d529de

DarshitChanpura · 2025-07-29T18:45:09Z

src/main/java/org/opensearch/timeseries/util/ParseUtils.java

-        // detectorId will be null when this is a create request and so we don't need resource authz check
-        if (shouldEvaluateWithNewAuthz && !Strings.isNullOrEmpty(configId)) {
-            ResourceSharingClient resourceSharingClient = ResourceSharingClientAccessor.getInstance().getResourceSharingClient();
-            resourceSharingClient.verifyAccess(configId, configIndex, ActionListener.wrap(isAuthorized -> {


with security plugin's lates change, access will be auto evaluated. Thus, we no longer need to explicitly call verifyAccess.

Okay so from my understanding from offline conversation:
verifyAccess will look for a few things currently:

global flag must be turned on along with ad flag

request must be of DocRequest type

any existing sharing info must have been migrated to the new model using /migrate API offered by security plugin

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura · 2025-08-05T00:06:51Z

CI will be unblocked once opensearch-project/security#5541 merges.

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

amitgalitz · 2025-08-05T20:25:05Z

src/main/java/org/opensearch/ad/transport/AnomalyResultRequest.java


    public AnomalyResultRequest(String adID, long start, long end) {
-        super(adID, start, end);
+        super(adID, ADIndex.CONFIG.getIndexName(), start, end);


why are we passing the AD config index here, is it cause we are only checking resource permission on config, we don't care for the other system indices right?

this is required for auto Resource Request evaluation. The base class ResultRequest extends DocRequest interface which require methods index() and id() to be implemented. These values are then used in security plugin to determine the resource id and index where resource resides, and use that information to perform evaluation.

codecov · 2025-08-05T20:25:14Z

Codecov Report

❌ Patch coverage is 61.40351% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.53%. Comparing base (50a2819) to head (ca4e0b3).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...ch/timeseries/transport/ValidateConfigRequest.java	0.00%	4 Missing ⚠️
...nsearch/timeseries/transport/GetConfigRequest.java	57.14%	3 Missing ⚠️
...ava/org/opensearch/timeseries/util/ParseUtils.java	0.00%	1 Missing and 2 partials ⚠️
...ch/ad/transport/PreviewAnomalyDetectorRequest.java	0.00%	2 Missing ⚠️
...arch/timeseries/transport/DeleteConfigRequest.java	66.66%	2 Missing ⚠️
...rg/opensearch/timeseries/transport/JobRequest.java	66.66%	2 Missing ⚠️
...opensearch/timeseries/transport/ResultRequest.java	60.00%	2 Missing ⚠️
...arch/forecast/rest/RestDeleteForecasterAction.java	0.00%	1 Missing ⚠️
...ansport/BaseSuggestConfigParamTransportAction.java	75.00%	1 Missing ⚠️
...s/transport/BaseValidateConfigTransportAction.java	66.66%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1533      +/-   ##
============================================
- Coverage     81.59%   81.53%   -0.06%     
+ Complexity     6033     6031       -2     
============================================
  Files           536      536              
  Lines         24427    24427              
  Branches       2494     2494              
============================================
- Hits          19930    19917      -13     
- Misses         3260     3271      +11     
- Partials       1237     1239       +2

Flag	Coverage Δ
plugin	`81.53% <61.40%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...ensearch/ad/rest/RestAnomalyDetectorJobAction.java	`100.00% <100.00%> (ø)`
...earch/ad/rest/RestDeleteAnomalyDetectorAction.java	`100.00% <100.00%> (ø)`
...ensearch/ad/rest/RestGetAnomalyDetectorAction.java	`100.00% <100.00%> (ø)`
...est/handler/IndexAnomalyDetectorActionHandler.java	`100.00% <ø> (ø)`
...d/transport/AnomalyDetectorJobTransportAction.java	`100.00% <ø> (ø)`
.../opensearch/ad/transport/AnomalyResultRequest.java	`68.00% <100.00%> (ø)`
...arch/ad/transport/IndexAnomalyDetectorRequest.java	`100.00% <ø> (ø)`
...transport/IndexAnomalyDetectorTransportAction.java	`86.66% <ø> (-0.35%)`	⬇️
...ansport/PreviewAnomalyDetectorTransportAction.java	`76.04% <ø> (-0.49%)`	⬇️
...ansport/SearchTopAnomalyResultTransportAction.java	`92.16% <100.00%> (+0.04%)`	⬆️
... and 23 more

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

amitgalitz · 2025-08-05T20:29:16Z

.../java/org/opensearch/action/admin/indices/mapping/get/IndexForecasterActionHandlerTests.java

        }));
        assertTrue(inProgressLatch.await(10, TimeUnit.SECONDS));
    }
-


you removed this test, are we checking this somewhere else?

this branch is no longer being tested here since AD no longer has knowledge of feature flag OPENSEARCH_RESOURCE_SHARING_ENABLED. It was added here specifically to cover that scenario.

if you look index IndexForecasterActionHandler.java, you will see that I've remove relevant code as well. We no longer automatically share the resource with backend_roles upon creation, with the new authz scheme. Users will be required to explicitly share the resource.
A share API is en-route to be added to Security plugin.

AD also has an option to add its own /share api however security is the ideal place as it avoids multiple implementations by different plugins to achieve same result.

DarshitChanpura · 2025-08-05T20:41:56Z

src/main/java/org/opensearch/timeseries/util/ParseUtils.java

     * @return true if the resource-sharing feature and filter-by is enabled, false otherwise.
     */
    public static boolean shouldUseResourceAuthz(Settings settings) {
        boolean filterByEnabled = AnomalyDetectorSettings.AD_FILTER_BY_BACKEND_ROLES.get(settings);


the new authz feature is still toggleable by the old setting of filter_by_backend_role. However, the ask from AD maintainers is whether this should still be kept as a switch for turning on the new authz scheme?

yes I wanted to understand if we should be deprecating this switch already or not. From my understanding is for now:
global flag + ad flag -> new authz route
only ad flag -> current ad authz
no flag -> no sharing concept, default behavior where users can see other configs

…-auth

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

…es in the rest tests Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

opensearch-trigger-bot bot added the backport 2.x label Jul 29, 2025

DarshitChanpura commented Jul 29, 2025

View reviewed changes

DarshitChanpura changed the title ~~Onboards to centralized resource authz~~ Onboards to centralized resource access control for detectors and forecasters Jul 29, 2025

amitgalitz mentioned this pull request Aug 4, 2025

bumping gradle and nebula versions #1537

Merged

1 task

DarshitChanpura added 2 commits August 4, 2025 18:33

Onboards to the auto authz for Resource requests

24e1e27

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Fixes compilation issues

1e2ee9c

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura force-pushed the onboard-centralized-auth branch from 88d1bd0 to 1e2ee9c Compare August 4, 2025 22:52

DarshitChanpura added 2 commits August 5, 2025 14:09

Adds missing streamoutput write for configIndex fiedl

4a80cb1

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Update SuggestConfigParamRequestTests to bring coverage to 0.60

f2c3bde

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura marked this pull request as ready for review August 5, 2025 20:09

DarshitChanpura requested review from VijayanB, amitgalitz, dbwiddis, jackiehanyang, jmazanec15, jngz-es, joshpalis, kaituo, ohltyler, owaiskazi19, saratvemulapalli, sean-zheng-amazon, vamshin and ylwu-amzn as code owners August 5, 2025 20:09

amitgalitz reviewed Aug 5, 2025

View reviewed changes

DarshitChanpura commented Aug 5, 2025

View reviewed changes

DarshitChanpura removed the backport 2.x label Aug 5, 2025

DarshitChanpura added 4 commits August 5, 2025 18:17

Merge remote-tracking branch 'upstream/main' into onboard-centralized…

2448c96

…-auth

Undo config indexing request to not be a docRequest

c3d4ee5

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Disabling run with feature flag as it requires decent number of chang…

a17065c

…es in the rest tests Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Bump jacoco to 0.8.13

ca4e0b3

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

amitgalitz approved these changes Aug 6, 2025

View reviewed changes

amitgalitz merged commit e0e83ac into opensearch-project:main Aug 6, 2025
24 of 25 checks passed

Uh oh!

Onboards to centralized resource access control for detectors and forecasters #1533

Onboards to centralized resource access control for detectors and forecasters #1533

Uh oh!

Conversation

DarshitChanpura commented Jul 29, 2025

Description

Check List

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura commented Aug 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Aug 5, 2025 •

edited

Loading