Check if AD backend role is enabled #968

amitgalitz · 2023-06-22T16:12:25Z

Description of changes:
Right now the ad backend role filtering is always enabled, this is adding a check to make sure that AD backend role filtering is either disabled or enabled and apply that filtering correctly when retrieving detectors or calling on the result index in AD.

CheckList:

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.

Signed-off-by: Amit Galitzky <[email protected]>

getsaurabh02 · 2023-07-07T18:27:21Z

alerting/src/main/kotlin/org/opensearch/alerting/InputService.kt

@@ -209,7 +207,9 @@ class InputService(
                // Monitor runner will send transport request to check permission first. If security plugin response
                // is yes, user has permission to query AD result. If AD role filter enabled, we will add user role
                // filter to protect data at user role level; otherwise, user can query any AD result.
-                addUserBackendRolesFilter(monitor.user, searchRequest.source())
+                if (getADBackendRoleFilterEnabled(clusterService, settings)) {


Can getADBackendRoleFilterEnabled should be made more generic like getClientPluginRoleFilter so that we can supply the AD filter path just for that one use but the method itself can be used for other setting check?

made this change

getsaurabh02

Thanks LGTM overall! Added a suggestion on how we can de-couple the AD setting knowledge from the Alerting execution flow.

Signed-off-by: Amit Galitzky <[email protected]>

amitgalitz · 2023-07-10T20:56:55Z

I made the same code changes on a 2.8 branch and everything passed: amitgalitz@00c326a
Both locally and in above github link you can go to the green check mark for that commit

codecov · 2023-07-10T21:01:45Z

Codecov Report

Merging #968 (bf2c90f) into main (62d2524) will decrease coverage by 0.24%.
The diff coverage is 38.88%.

@@             Coverage Diff              @@
##               main     #968      +/-   ##
============================================
- Coverage     75.05%   74.81%   -0.24%     
+ Complexity      111      110       -1     
============================================
  Files           144      144              
  Lines          8341     8358      +17     
  Branches       1213     1223      +10     
============================================
- Hits           6260     6253       -7     
- Misses         1465     1485      +20     
- Partials        616      620       +4

Impacted Files	Coverage Δ
.../alerting/transport/TransportIndexMonitorAction.kt	`55.86% <0.00%> (-2.96%)`	⬇️
...tlin/org/opensearch/alerting/util/AlertingUtils.kt	`52.63% <22.22%> (-5.71%)`	⬇️
...ain/kotlin/org/opensearch/alerting/InputService.kt	`89.88% <60.00%> (-1.98%)`	⬇️
...n/kotlin/org/opensearch/alerting/AlertingPlugin.kt	`94.27% <100.00%> (ø)`

... and 10 files with indirect coverage changes

getsaurabh02

LGTM. Can we link the test success link.

amitgalitz · 2023-07-10T22:14:35Z

amitgalitz/alerting@00c326a

LGTM. Can we link the test success link.

https://github.com/amitgalitz/alerting/actions/runs/5512253293/jobs/10048841565

opensearch-trigger-bot · 2023-07-10T22:19:30Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-968-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 5fc607bf6f8137fd0ac7fba4522fcedd67e29cbc
# Push it to GitHub
git push --set-upstream origin backport/backport-968-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-968-to-2.x.

* check if ad filter is enabled Signed-off-by: Amit Galitzky <[email protected]> * updated integ tests Signed-off-by: Amit Galitzky <[email protected]> --------- Signed-off-by: Amit Galitzky <[email protected]>

* check if ad filter is enabled * updated integ tests --------- Signed-off-by: Amit Galitzky <[email protected]>

opensearch-trigger-bot · 2023-07-11T04:04:06Z

The backport to 2.9 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.9 2.9
# Navigate to the new working tree
cd .worktrees/backport-2.9
# Create a new branch
git switch --create backport/backport-968-to-2.9
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 5fc607bf6f8137fd0ac7fba4522fcedd67e29cbc
# Push it to GitHub
git push --set-upstream origin backport/backport-968-to-2.9
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.9

Then, create a pull request where the base branch is 2.9 and the compare/head branch is backport/backport-968-to-2.9.

* check if ad filter is enabled * updated integ tests --------- Signed-off-by: Amit Galitzky <[email protected]> (cherry picked from commit 62d3d7e)

* check if ad filter is enabled * updated integ tests --------- Signed-off-by: Amit Galitzky <[email protected]> (cherry picked from commit 62d3d7e) Co-authored-by: Amit Galitzky <[email protected]>

amitgalitz requested review from lezzago, AWSHurneyt, sbcd90, eirsep, getsaurabh02, praveensameneni, qreshi, bowenlan-amzn and rishabhmaurya as code owners June 22, 2023 16:12

check if ad filter is enabled

25df4be

Signed-off-by: Amit Galitzky <[email protected]>

amitgalitz force-pushed the filter-ad-bug branch from 70878cf to 25df4be Compare June 30, 2023 20:38

getsaurabh02 reviewed Jul 7, 2023

View reviewed changes

updated integ tests

bf2c90f

Signed-off-by: Amit Galitzky <[email protected]>

getsaurabh02 approved these changes Jul 10, 2023

View reviewed changes

lezzago added the backport 2.x label Jul 10, 2023

lezzago approved these changes Jul 10, 2023

View reviewed changes

lezzago merged commit 5fc607b into opensearch-project:main Jul 10, 2023
13 of 17 checks passed

AWSHurneyt pushed a commit that referenced this pull request Jul 11, 2023

Check if AD backend role is enabled (#968) (#989)

62d3d7e

* check if ad filter is enabled * updated integ tests --------- Signed-off-by: Amit Galitzky <[email protected]>

AWSHurneyt added the backport 2.9 backports PRs to 2.9 label Jul 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if AD backend role is enabled #968

Check if AD backend role is enabled #968

amitgalitz commented Jun 22, 2023

getsaurabh02 Jul 7, 2023 •

edited

Loading

amitgalitz Jul 10, 2023

getsaurabh02 left a comment •

edited

Loading

amitgalitz commented Jul 10, 2023

codecov bot commented Jul 10, 2023

getsaurabh02 left a comment

amitgalitz commented Jul 10, 2023

opensearch-trigger-bot bot commented Jul 10, 2023

opensearch-trigger-bot bot commented Jul 11, 2023

Check if AD backend role is enabled #968

Check if AD backend role is enabled #968

Conversation

amitgalitz commented Jun 22, 2023

getsaurabh02 Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

amitgalitz Jul 10, 2023

Choose a reason for hiding this comment

getsaurabh02 left a comment • edited Loading

Choose a reason for hiding this comment

amitgalitz commented Jul 10, 2023

codecov bot commented Jul 10, 2023

Codecov Report

getsaurabh02 left a comment

Choose a reason for hiding this comment

amitgalitz commented Jul 10, 2023

opensearch-trigger-bot bot commented Jul 10, 2023

opensearch-trigger-bot bot commented Jul 11, 2023

getsaurabh02 Jul 7, 2023 •

edited

Loading

getsaurabh02 left a comment •

edited

Loading