Add filter by backend roles access strategy setting by markdboyd · Pull Request #1146 · opensearch-project/notifications

markdboyd · 2026-03-11T18:57:41Z

Description

This PR adds a new plugin setting, plugins.notifications.general.filter_by_backend_roles_access_strategy, which allows users to control how filtering by backend roles works to determine access to notification objects (e.g. SMTP senders, email recipient groups, channels). The options for this setting are:

exact - Users have access to alerting objects if they have exactly the same (with no additional) backend roles as the user who created the object
intersect - Users have access to alerting objects if they share at least one backend role with the user who created the object
all - Users have access to alerting objects if their backend roles contain all of the backend roles of the user who created the object

Related Issues

Closes #1079

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.

markdboyd · 2026-03-11T19:05:16Z

+        }
+    }
+
+    fun `test get smtp sender has access with filter by backend roles enabled`() {


I think we need the integration tests from here onwards for at least config type to prove that access controls work as expected when filter_by_backend_roles: true, including in conjunction with the filter_by_backend_roles_access_strategy setting..

I'm debating whether we need to replicate these tests for other config objects (email groups, channels, etc), or whether these tests plus the tests in UserAccessManagerTests.kt are sufficient

Ideally, it would be nice to have that coverage.

In my opinion though, the higher priority would be having integ test coverage for editing an asset with/without the necessary backend roles (example edit channel test), deleting an asset with/without the necessary backend roles (example delete channel test), and sending test notification messages with/without the necessary backend roles (example send test message test).

Properly filtering out assets (e.g., channel configs) based on backend roles should prevent a user from performing those actions via the UI, but they could still try by using curl commands/devtools if they happen to know the ID for the asset.

AWSHurneyt · 2026-03-25T22:58:17Z

+    /**
+     * Backend roles must be exactly equal to have access
+     */
+    ALL("all")


For visibility to other reviewers, we're discussing adjusting the name of this strategy away from ALL in this thread.
opensearch-project/alerting#2034 (comment)

AWSHurneyt · 2026-03-31T00:13:03Z

@markdboyd could you rebase your branch from opensearch-project:main, and resolve the conflicts? I'd like to see whether the checks pass.

markdboyd · 2026-03-31T20:12:50Z

@AWSHurneyt - OK, I've rebased from https://github.com/opensearch-project/notifications/tree/main

AWSHurneyt

Side note:
The PR description could use a little updating to reference the 3 strategies and their function.

AWSHurneyt · 2026-06-10T16:21:05Z

+        }
+    }
+
+    fun `test get smtp sender has access with filter by backend roles enabled`() {


Ideally, it would be nice to have that coverage.

In my opinion though, the higher priority would be having integ test coverage for editing an asset with/without the necessary backend roles (example edit channel test), deleting an asset with/without the necessary backend roles (example delete channel test), and sending test notification messages with/without the necessary backend roles (example send test message test).

Properly filtering out assets (e.g., channel configs) based on backend roles should prevent a user from performing those actions via the UI, but they could still try by using curl commands/devtools if they happen to know the ID for the asset.

markdboyd · 2026-06-12T16:33:26Z

@AWSHurneyt OK, I added tests for:

editing and deleting SMTP account with and without access
creating, getting, editing, and deleting an email group without access
sending slack notification messages with and without access

Is there additional test coverage that you would like me to add?

…access strategy is exact Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

…ies for get and update operations on email notification channels Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

…ests Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

cwperks · 2026-07-08T01:39:31Z

@markdboyd apologies for the delay on getting comments to this PR and the alerting PR. Overall the code changes look good to me and it makes sense to adding this setting to working with filter_by_backend_roles for any cluster admin wanting to use backend roles to control visibility of resources created from the notifications and alerting plugins. Along with this PR we should also submit PRs to the https://github.com/opensearch-project/documentation-website repo to document the new plugin settings.

cwperks · 2026-07-08T01:41:59Z

+        DEFAULT_FILTER_BY_BACKEND_ROLES_ACCESS_STRATEGY,
+        FilterByBackendRolesAccessStrategyValidator(),
+        NodeScope,
+        Dynamic


@markdboyd Let's also add Sensitive into this vararg list of settings properties. Sensitive would mean that the cluster setting is only toggleable by a security admin vs. any user w/ cluster:admin/settings/put. Its not quite fine grained access to settings, but its a 2 tier system to differentiate between security admin vs other users.

cwperks · 2026-07-08T01:44:10Z

+                RestStatus.OK.status,
+                updateUserClient
+            )
+            Thread.sleep(1000)


Can we use Awaitility (or similar for Kotlin) to wait for condition and avoid Thread.sleep?

I opted to just remove the Thread.sleep and it seems like the tests are still passing. I suspect that they were put in place to handle some race conditions, but perhaps they are not necessary anymore

cwperks · 2026-07-08T01:45:46Z

+        try {
+            val configId = createConfigWithRequestJsonString(createRequestJsonString, userClient!!)
+            Assert.assertNotNull(configId)
+            Thread.sleep(1000)


Same here, prefer Awaitility to Thread.sleep

cwperks · 2026-07-08T01:47:36Z

+        val (emailGroupConfig, createEmailGroupRequestJsonString) = createTestEmailGroup()
+
+        val getUser = "getUser"
+        val getUserClient = SecureRestClientBuilder(clusterHosts.toTypedArray(), isHttps(), getUser, password)


I see a lot of this code repeated. Not a blocker for these PRs, but I'd love to extract this out to a helper method where you can supply username, password and get a client...or get a client and pass username and password at the request level.

Addressed in 823f1d0. I did notice that there was a buildClient helper function already: https://github.com/opensearch-project/notifications/blob/main/notifications/notifications/src/test/kotlin/org/opensearch/integtest/PluginRestTestCase.kt#L108. However, since it reads the username/password via System.getProperty, it didn't seem usable in these integration tests where we need to create a client based on a dynamic username/password.

…ude the Sensitive property Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

…estClient Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

markdboyd · 2026-07-13T20:41:01Z

@cwperks Here is the related documentation changes: opensearch-project/documentation-website#12794

AWSHurneyt · 2026-07-20T18:20:01Z

Confirmed with @cwperks offline that his comments have been addressed. Merging.

markdboyd requested review from Hailong-am, SuZhou-Joe, amsiglan, gaobinlong, praveensameneni, sbcd90, toepkerd, xluo-aws, yuye-aws, zhichao-aws and zhongnansu as code owners March 11, 2026 18:57

markdboyd force-pushed the add-filter-by-access-strategy-setting branch from 11e01f5 to 306f22e Compare March 11, 2026 18:58

markdboyd commented Mar 11, 2026

View reviewed changes

AWSHurneyt reviewed Mar 31, 2026

View reviewed changes

markdboyd force-pushed the add-filter-by-access-strategy-setting branch 2 times, most recently from 0a0d24e to 998d8d3 Compare March 31, 2026 20:12

markdboyd force-pushed the add-filter-by-access-strategy-setting branch from fe46f99 to 9e2f308 Compare June 9, 2026 13:37

markdboyd requested review from KashKondaka, eirsep, manaswini1920, nishtham-amazon, riysaxen-amzn and vikhy-aws as code owners June 9, 2026 13:37

AWSHurneyt reviewed Jun 10, 2026

View reviewed changes

markdboyd force-pushed the add-filter-by-access-strategy-setting branch from 164aacf to b475a49 Compare June 12, 2026 16:31

markdboyd force-pushed the add-filter-by-access-strategy-setting branch 2 times, most recently from 9134c35 to 82bce43 Compare June 17, 2026 21:02

markdboyd added 10 commits June 18, 2026 16:12

add tests for slack notification access when filter by backend roles …

4fb62bd

…access strategy is exact Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

stub out access tests for email groups

f3cf157

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

reorganize test files

19cac47

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

refactoring email group access tests

eb40395

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

add more integration tests for email group access

69b8ad5

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

create stub of integration test for access to email notifications

4c5a85f

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

add more tests for email notification access

55d8f6e

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

fix test logic for email notification access integration tests

65c26a1

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

add access tests with different filter by backend role access strateg…

b90b4bd

…ies for get and update operations on email notification channels Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

add delete access integration tests for email notification channels

d23adaf

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

markdboyd force-pushed the add-filter-by-access-strategy-setting branch from 82bce43 to d23adaf Compare June 18, 2026 20:12

AWSHurneyt reviewed Jun 26, 2026

View reviewed changes

markdboyd added 2 commits June 29, 2026 10:18

address feedback on integration tests

75c2630

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

address feedback on FilterByBackendRolesAccessStrategyValidator and t…

8f7c2d7

…ests Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

cwperks reviewed Jul 8, 2026

View reviewed changes

markdboyd added 4 commits July 13, 2026 11:30

update FILTER_BY_BACKEND_ROLES_ACCESS_STRATEGY plugin setting to incl…

e809bf8

…ude the Sensitive property Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

remove usage of Thread.sleep in integration tests

e7ab4fc

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

update notifications integration tests to use helper for building a R…

823f1d0

…estClient Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

remove Thread.sleep from integration tests

d9df668

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

markdboyd mentioned this pull request Jul 13, 2026

Document new filter by backend roles strategy settings for notifications and alerting plugins opensearch-project/documentation-website#12794

Merged

1 task

AWSHurneyt approved these changes Jul 15, 2026

View reviewed changes

AWSHurneyt merged commit c013118 into opensearch-project:main Jul 20, 2026
14 checks passed

markdboyd deleted the add-filter-by-access-strategy-setting branch July 20, 2026 18:37

opensearch-ci-bot mentioned this pull request Jul 22, 2026

[AUTO] Add release notes for 3.8.0 #1246

Merged

Uh oh!

Conversation

markdboyd commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

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!

AWSHurneyt commented Mar 31, 2026

Uh oh!

markdboyd commented Mar 31, 2026

Uh oh!

AWSHurneyt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markdboyd commented Jun 12, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cwperks commented Jul 8, 2026

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!

markdboyd commented Jul 13, 2026

Uh oh!

AWSHurneyt commented Jul 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

markdboyd commented Mar 11, 2026 •

edited

Loading