Updates search handler to consume resource authz and updates resource authz related tests #1546

DarshitChanpura · 2025-08-12T00:35:53Z

Description

[Describe what this change achieves]

Related Issues

Resolves #[Issue number to be closed when this PR 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.

Signed-off-by: Darshit Chanpura <[email protected]>

codecov · 2025-08-13T18:55:13Z

Codecov Report

❌ Patch coverage is 41.48936% with 55 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.42%. Comparing base (17e56f0) to head (2a4fadf).

Files with missing lines	Patch %	Lines
...ava/org/opensearch/timeseries/util/ParseUtils.java	14.81%	20 Missing and 3 partials ⚠️
...transport/DeleteAnomalyResultsTransportAction.java	5.55%	17 Missing ⚠️
...arch/ad/transport/IndexAnomalyDetectorRequest.java	0.00%	2 Missing ⚠️
...rch/forecast/transport/IndexForecasterRequest.java	0.00%	2 Missing ⚠️
...ch/timeseries/transport/handler/SearchHandler.java	50.00%	1 Missing and 1 partial ⚠️
...transport/IndexAnomalyDetectorTransportAction.java	50.00%	1 Missing ⚠️
...ansport/PreviewAnomalyDetectorTransportAction.java	50.00%	1 Missing ⚠️
...cast/transport/ForecastRunOnceTransportAction.java	50.00%	1 Missing ⚠️
...cast/transport/IndexForecasterTransportAction.java	88.88%	1 Missing ⚠️
.../transport/SearchForecastTasksTransportAction.java	0.00%	1 Missing ⚠️
... and 4 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1546      +/-   ##
============================================
- Coverage     81.59%   81.42%   -0.18%     
- Complexity     6039     6041       +2     
============================================
  Files           536      536              
  Lines         24427    24474      +47     
  Branches       2494     2500       +6     
============================================
- Hits          19932    19928       -4     
- Misses         3260     3309      +49     
- Partials       1235     1237       +2

Flag	Coverage Δ
plugin	`81.42% <41.48%> (-0.18%)`	⬇️

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

Files with missing lines	Coverage Δ
.../java/org/opensearch/ad/model/AnomalyDetector.java	`89.93% <100.00%> (+0.06%)`	⬆️
...rch/ad/transport/SearchADTasksTransportAction.java	`100.00% <100.00%> (ø)`
...ransport/SearchAnomalyDetectorTransportAction.java	`100.00% <100.00%> (ø)`
.../transport/SearchAnomalyResultTransportAction.java	`86.66% <100.00%> (ø)`
...ansport/SearchTopAnomalyResultTransportAction.java	`92.16% <100.00%> (ø)`
...java/org/opensearch/forecast/model/Forecaster.java	`80.22% <100.00%> (-0.92%)`	⬇️
...nsport/SearchTopForecastResultTransportAction.java	`66.12% <100.00%> (ø)`
...n/java/org/opensearch/timeseries/model/Config.java	`86.92% <100.00%> (+0.04%)`	⬆️
...ies/transport/BaseDeleteConfigTransportAction.java	`83.63% <100.00%> (+0.75%)`	⬆️
...h/timeseries/transport/BaseJobTransportAction.java	`90.00% <100.00%> (+2.50%)`	⬆️
... and 14 more

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

Signed-off-by: Darshit Chanpura <[email protected]>

…ld in config xcontent parser for validate requests and correct jvm debugger args Signed-off-by: Darshit Chanpura <[email protected]>

Signed-off-by: Darshit Chanpura <[email protected]>

This reverts commit fe6aae8. Signed-off-by: Darshit Chanpura <[email protected]>

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura · 2025-08-20T16:50:40Z

src/main/java/org/opensearch/ad/model/AnomalyDetector.java

@@ -465,6 +465,11 @@ public static AnomalyDetector parse(
            parser.nextToken();

            switch (fieldName) {
+                case ID_FIELD:


needed for access control on Validate method.

Would there be a privilege escalation when one user writes someone else's id in the request body?

No they cannot, all resource related requests are first checked for permission here, before proceeding to execute it:
https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/privileges/ResourceAccessEvaluator.java#L67

DarshitChanpura · 2025-08-20T16:51:56Z

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

@@ -145,4 +147,13 @@ public ActionRequestValidationException validate() {
        return null;
    }

+    @Override
+    public String index() {


this handles the Update scenario.

DarshitChanpura · 2025-08-20T17:19:06Z

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

-                ),
-                new Object[] {},
-                (fallbackArgs) -> resolveUserAndExecute(
+                () -> indexDetector(user, detectorId, method, listener, detector -> adExecute(request, user, detector, context, listener)),


most of these changes around verifyResourceAccessAndProcessRequest are code cleanup.

DarshitChanpura · 2025-08-20T17:20:57Z

src/main/java/org/opensearch/timeseries/transport/handler/SearchHandler.java

     * @param actionListener action listerner
     */
-    public void search(SearchRequest request, ActionListener<SearchResponse> actionListener) {
+    public void search(SearchRequest request, Pair<String, String> pair, ActionListener<SearchResponse> actionListener) {


please review this. This change currently modifies all searchHandler.search calls to pass the index name and id field of the config. However, the search calls that are made from tasks and results indices are not covered by resource-authz. Only AD and forecast config index are, since those two are registered as protected indices.

DarshitChanpura · 2025-08-20T17:21:37Z

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

+        logger.debug("Filtering result by accessible resources");
+        ResourceSharingClient resourceSharingClient = ResourceSharingClientAccessor.getInstance().getResourceSharingClient();
+        SearchSourceBuilder searchSourceBuilder = searchRequest.source();
+        resourceSharingClient.getAccessibleResourceIds(pair.getLeft(), ActionListener.wrap(configIds -> {


makes as call to get accessible Id and then adds a filter to current search query to only search from those ids.

DarshitChanpura · 2025-08-20T17:25:24Z

src/test/java/org/opensearch/forecast/rest/SecureForecastRestIT.java

+        // case 5: given a forecaster Id, read access user can
+        if (isResourceSharingFeatureEnabled()) {
+            // not look up forecaster configuration for full-client's forecaster
+            exception = expectThrows(


user won't have access by default.

DarshitChanpura · 2025-08-20T17:25:52Z

src/test/java/org/opensearch/forecast/rest/SecureForecastRestIT.java

@@ -1471,6 +1627,309 @@ public void testFilterBy() throws IOException {
        }
    }

+    public void testResourceSharing() throws IOException {


adds test specifically for resource sharing feature

DarshitChanpura · 2025-08-20T17:26:47Z

src/test/java/org/opensearch/ad/rest/SecureADRestIT.java

+        }, notNullValue());
+    }
+
+    private void waitForRevokeNonVisibility(String detId, RestClient client) {


to test that resource is no longer visible.

DarshitChanpura · 2025-08-20T17:27:01Z

src/test/java/org/opensearch/ad/rest/SecureADRestIT.java

    String oceanUser = "ocean";
    RestClient oceanClient;

+    private void waitForSharingVisibility(String detId, RestClient client) {


to test that resource is visible.

DarshitChanpura · 2025-08-20T17:27:30Z

src/test/java/org/opensearch/ad/rest/SecureADRestIT.java

-            .assertTrue(
-                exception.getMessage().contains("Filter by backend roles is enabled and User dog does not have backend roles configured")
-            );
+        if (isResourceSharingFeatureEnabled()) {


covers resource sharing feature enabled scenario.

kaituo

read until src/main/java/org/opensearch/ad/transport/IndexAnomalyDetectorRequest.java

kaituo · 2025-08-21T17:35:34Z

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

@@ -70,19 +79,46 @@ public void delete(DeleteByQueryRequest request, ActionListener<BulkByScrollResp
    }

    private void validateRole(DeleteByQueryRequest request, User user, ActionListener<BulkByScrollResponse> listener) {
-        if (user == null || !filterEnabled) {
+        if (user == null || !(filterEnabled || shouldUseResourceAuthz)) {


Can you update comment as you add a new condition?

kaituo · 2025-08-21T17:37:44Z

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

-                ParseUtils.addUserBackendRolesFilter(user, request.getSearchRequest().source());
+                // Security is enabled and resource sharing access control is enabled
+                if (shouldUseResourceAuthz) {
+                    // verify if new authz should be added on result index, if soo replace the null


kaituo · 2025-08-21T18:12:24Z

src/main/java/org/opensearch/ad/model/AnomalyDetector.java

@@ -465,6 +465,11 @@ public static AnomalyDetector parse(
            parser.nextToken();

            switch (fieldName) {
+                case ID_FIELD:


Would there be a privilege escalation when one user writes someone else's id in the request body?

kaituo · 2025-08-21T18:16:03Z

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

            try {
-                ParseUtils.addUserBackendRolesFilter(user, request.getSearchRequest().source());
+                // Security is enabled and resource sharing access control is enabled
+                if (shouldUseResourceAuthz) {


What if both shouldUseResourceAuthz and filterEnabled are true? In your current code, would that increase a user's privilege as you only check shouldUseResourceAuthz? This can happen in an old cluster when backend role filtering is enabled.

we don't intend for mixed cluster usage. If both are enabled, resource-authz takes precedence. At that point, cluster-admin is expected to have migrated resources from old sharing model to the new one.

kaituo · 2025-08-21T19:35:07Z

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

+                if (shouldUseResourceAuthz) {
+                    // verify if new authz should be added on result index, if soo replace the null
+                    addAccessibleConfigsFilterAndDelete(ADIndex.RESULT.getIndexName(), request.getSearchRequest(), listener);
+                    return;


Is using a terms filter (ideally via terms lookup) directly inside Delete-By-Query is better than a separate “fetch IDs → inject into DBQ” step? Terms filter keeps authorization and deletion in a single request, eliminating the read-then-delete” race where ACLs/ownership can change between calls. DBQ will return version conflict error during race condition (see https://discuss.elastic.co/t/concurrent-delete-by-query-and-indexing/68868/2
). Also, writing a single DBQ evaluates the latest ACLs at request time and enforces least-privilege by intersecting resource-based authz with any backend-role filter in the same bool query—so enabling both cannot widen access. It also removes a network round trip, reduces latency and error surface, and keeps the request small—no 1,000-ID terms array bloating the query body—while avoiding large in-memory ID lists in the transport layer. The control flow becomes linear (assemble filters → run one DBQ), easier to read, test, and maintain, and if no resources are accessible the filter simply matches none (fail-closed).

before I answer this is ADResult Index going to be a protected index? if so we should mark is as such in TimeSeriesResourceSharingExtension.

…cally, adds resource-action-groups yml for sharing and pluginClient for search Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura

CI is expected to fail until opensearch-project/security#5597 is merged.

SearchWrapper for resource-sharing requests is now being handled DLS style and will be available once this PR merges: opensearch-project/security#5600

DarshitChanpura · 2025-09-09T03:08:32Z

src/main/java/org/opensearch/ad/model/AnomalyDetector.java

@@ -465,6 +465,11 @@ public static AnomalyDetector parse(
            parser.nextToken();

            switch (fieldName) {
+                case ID_FIELD:


No they cannot, all resource related requests are first checked for permission here, before proceeding to execute it:
https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/privileges/ResourceAccessEvaluator.java#L67

DarshitChanpura · 2025-09-09T03:15:03Z

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

@@ -70,19 +79,46 @@ public void delete(DeleteByQueryRequest request, ActionListener<BulkByScrollResp
    }

    private void validateRole(DeleteByQueryRequest request, User user, ActionListener<BulkByScrollResponse> listener) {
-        if (user == null || !filterEnabled) {
+        if (user == null || !(filterEnabled || shouldUseResourceAuthz)) {


DarshitChanpura · 2025-09-09T03:16:02Z

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

            try {
-                ParseUtils.addUserBackendRolesFilter(user, request.getSearchRequest().source());
+                // Security is enabled and resource sharing access control is enabled
+                if (shouldUseResourceAuthz) {


we don't intend for mixed cluster usage. If both are enabled, resource-authz takes precedence. At that point, cluster-admin is expected to have migrated resources from old sharing model to the new one.

DarshitChanpura · 2025-09-09T03:18:10Z

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

+                if (shouldUseResourceAuthz) {
+                    // verify if new authz should be added on result index, if soo replace the null
+                    addAccessibleConfigsFilterAndDelete(ADIndex.RESULT.getIndexName(), request.getSearchRequest(), listener);
+                    return;


before I answer this is ADResult Index going to be a protected index? if so we should mark is as such in TimeSeriesResourceSharingExtension.

DarshitChanpura · 2025-09-09T04:09:56Z

src/main/java/org/opensearch/timeseries/transport/handler/SearchHandler.java

+        Settings settings,
+        ClusterService clusterService,
+        Client client,
+        PluginClient pluginClient,


PluginClient acts as a wrapper for search and security plugin will apply DLS automatically through it for resource-sharing requests

DarshitChanpura · 2025-09-09T04:10:47Z

src/main/resources/resource-action-groups.yml

@@ -0,0 +1,31 @@
+# no internal actions are included here
+resource_types:


please review this, as these are the action-groups that will be displayed on dashboard page for resource-access-management.

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura added 2 commits August 11, 2025 20:33

Updates tests

17c4367

Signed-off-by: Darshit Chanpura <[email protected]>

Merge remote-tracking branch 'upstream/main' into resource-authz-tests

839d93f

opensearch-trigger-bot bot added infra Changes to infrastructure, testing, CI/CD, pipelines, etc. backport 2.x labels Aug 12, 2025

Fix failing test for non-security workflow

5ea6079

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura force-pushed the resource-authz-tests branch from a084db8 to 5ea6079 Compare August 12, 2025 20:22

DarshitChanpura added 2 commits August 12, 2025 17:42

Fix AnomalyDetectorRestApiIT by adding awaitility

bea693d

Signed-off-by: Darshit Chanpura <[email protected]>

Fix test workflow

5410c0c

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura added 6 commits August 13, 2025 16:21

Refactors verifyAccess method

52765cf

Signed-off-by: Darshit Chanpura <[email protected]>

Merge remote-tracking branch 'upstream/main' into resource-authz-tests

4a5d408

Adapts changes to make ResourceExtension consumed via @Inject

fe6aae8

Signed-off-by: Darshit Chanpura <[email protected]>

Updates Index request to be DocRequest to handle updates, adds id fie…

e922cee

…ld in config xcontent parser for validate requests and correct jvm debugger args Signed-off-by: Darshit Chanpura <[email protected]>

Fix tests and adds tests for search API

73183f4

Signed-off-by: Darshit Chanpura <[email protected]>

Revert "Adapts changes to make ResourceExtension consumed via @Inject"

7596e9b

This reverts commit fe6aae8. Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura force-pushed the resource-authz-tests branch from de3cae6 to 7596e9b Compare August 19, 2025 19:36

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

DarshitChanpura added 4 commits August 19, 2025 17:21

Addresses test failures when feature flag is enabled

f985157

Signed-off-by: Darshit Chanpura <[email protected]>

Adds changelog entry and fixes extension over-ridden method

de59ded

Signed-off-by: Darshit Chanpura <[email protected]>

Adds ResourceSharing tests for AD

9f17422

Signed-off-by: Darshit Chanpura <[email protected]>

Adds ResourceSharing tests for Forecaster

e336e08

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura marked this pull request as ready for review August 20, 2025 16:49

DarshitChanpura requested review from jmazanec15, jngz-es, kaituo, saratvemulapalli, ohltyler, vamshin, VijayanB and ylwu-amzn as code owners August 20, 2025 16:49

DarshitChanpura requested review from amitgalitz, jackiehanyang, sean-zheng-amazon, dbwiddis, owaiskazi19 and joshpalis as code owners August 20, 2025 16:49

Removes logger info in test

2a4fadf

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura commented Aug 20, 2025

View reviewed changes

kaituo reviewed Aug 21, 2025

View reviewed changes

Updates search calls to use pluginClient that will apply DLS automati…

787005c

…cally, adds resource-action-groups yml for sharing and pluginClient for search Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura commented Sep 9, 2025

View reviewed changes

DarshitChanpura added 2 commits September 9, 2025 00:18

Fix forbiddenApis failure

55e7dc9

Signed-off-by: Darshit Chanpura <[email protected]>

Registers plugin client as injectable component

8b3fd6e

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura force-pushed the resource-authz-tests branch from 434ca83 to 8b3fd6e Compare September 9, 2025 18:35

Adds plugin-additional-permissions.yml for plugin client

c2dd98e

Signed-off-by: Darshit Chanpura <[email protected]>

		@@ -0,0 +1,31 @@
		# no internal actions are included here
		resource_types:

Updates search handler to consume resource authz and updates resource authz related tests #1546

Are you sure you want to change the base?

Updates search handler to consume resource authz and updates resource authz related tests #1546

Uh oh!

Conversation

DarshitChanpura commented Aug 12, 2025

Description

Related Issues

Check List

Uh oh!

codecov bot commented Aug 13, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaituo left a comment

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!

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 left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Aug 13, 2025 •

edited

Loading