Admin Users must be able to access all monitors #139 by skkosuri-amzn · Pull Request #220 · opensearch-project/alerting

skkosuri-amzn · 2021-10-29T20:48:46Z

Issue #, if available:
#139

Passed security integ tests using OpenSeach-1.1 release: ./gradlew :alerting:integTest -Dtests.rest.cluster=localhost:9200 -Dtests.cluster=localhost:9200 -Dtests.clustername=opensearch -Dhttps=true -Dsecurity=true -Duser=admin -Dpassword=admin.

Description of changes:
#139

Added changes that will ensure that admin users can access all monitors.
Refactored the transport classes which have filter-by-backend-role logic.
There is scope to improve security tests added a seperate issue Refactor security integration tests #219 , as fast follow up to this.

CheckList:
[x] 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.

codecov-commenter · 2021-10-29T20:54:11Z

Codecov Report

Merging #220 (031d9fc) into main (e0e41df) will decrease coverage by 0.08%.
The diff coverage is 48.71%.

@@             Coverage Diff              @@
##               main     #220      +/-   ##
============================================
- Coverage     78.86%   78.77%   -0.09%     
  Complexity      214      214              
============================================
  Files           172      173       +1     
  Lines          6964     6959       -5     
  Branches        908      912       +4     
============================================
- Hits           5492     5482      -10     
- Misses          987      993       +6     
+ Partials        485      484       -1

Impacted Files	Coverage Δ
...tlin/org/opensearch/alerting/util/AlertingUtils.kt	`81.81% <ø> (+27.47%)`	⬆️
...search/alerting/transport/SecureTransportAction.kt	`30.76% <30.76%> (ø)`
...ting/transport/TransportDeleteDestinationAction.kt	`29.09% <50.00%> (-4.25%)`	⬇️
...alerting/transport/TransportDeleteMonitorAction.kt	`34.69% <50.00%> (-4.53%)`	⬇️
...ch/alerting/transport/TransportGetMonitorAction.kt	`78.04% <60.00%> (-3.77%)`	⬇️
...rch/alerting/transport/TransportGetAlertsAction.kt	`74.24% <75.00%> (-2.57%)`	⬇️
...erting/transport/TransportGetDestinationsAction.kt	`74.64% <75.00%> (-2.38%)`	⬇️
...alerting/transport/TransportSearchMonitorAction.kt	`54.16% <75.00%> (-7.38%)`	⬇️
...rting/transport/TransportIndexDestinationAction.kt	`60.40% <80.00%> (-0.79%)`	⬇️
.../alerting/transport/TransportIndexMonitorAction.kt	`60.00% <80.00%> (-0.38%)`	⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0e41df...031d9fc. Read the comment docs.

alerting/src/main/kotlin/org/opensearch/alerting/transport/SecureTransportAction.kt

qreshi · 2021-11-01T20:24:48Z

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportDeleteMonitorAction.kt


    init {
-        clusterService.clusterSettings.addSettingsUpdateConsumer(AlertingSettings.FILTER_BY_BACKEND_ROLES) { filterByEnabled = it }
+        listenFilterBySettingChange(clusterService)


Since this is a method defined in an interface and is being used as an init, should we make the method final to prevent unexpected behavior or is the expectation that this method be overridden in the future for more complicated inheritance hierarchies of SecureTransportAction?

I think this should be a var since this is a setting that can change overtime

I cant make listenFilterBySettingChange final since it is in an interface.

I don't think the AlertingSettings.FILTER_BY_BACKEND_ROLES will be changing too often.

If the user needs to specify that listener in the init manually for any child class anyway, it might be better to have them explicitly state it rather than leave listenFilterBySettingChange open to override since it could be misleading.

Alternatively, we could make the class abstract to make the method final. Just something to consider for longevity, I don't think it's a blocker to merge.

alerting/src/test/kotlin/org/opensearch/alerting/resthandler/SecureMonitorRestApiIT.kt

opensearch-project#220) * Admin Users must be able to access all monitors opensearch-project#139 * Refactored

* Update copyright notice (#222) Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Admin Users must be able to access all monitors #139 (#220) * Admin Users must be able to access all monitors #139 * Refactored Co-authored-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> Co-authored-by: Sriram <59816283+skkosuri-amzn@users.noreply.github.com>

* Admin Users must be able to access all monitors #139 (#220) * Admin Users must be able to access all monitors #139 * Refactored * Update copyright notice (#222) Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> Co-authored-by: Sriram <59816283+skkosuri-amzn@users.noreply.github.com> Co-authored-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>

* Cherry-pick commits to 1.x (#227) * Update copyright notice (#222) Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Admin Users must be able to access all monitors #139 (#220) * Admin Users must be able to access all monitors #139 * Refactored Co-authored-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> Co-authored-by: Sriram <59816283+skkosuri-amzn@users.noreply.github.com> * Updates alerting version to 1.2 (#192) * Updates alerting version to 1.2 * Adds snapshot repo to the repository file Signed-off-by: Clay Downs <downsrob@amazon.com> * Update build to use public Maven repo (#184) Signed-off-by: Abbas Hussain <abbas_10690@yahoo.com> * Publish notification JARs checksums. (#196) * Publish notification JARs checksums. Signed-off-by: dblock <dblock@dblock.org> * Remove sonatype staging. Signed-off-by: dblock <dblock@dblock.org> * Updates testCompile mockito version to match OpenSearch changes (#204) Signed-off-by: Clay Downs <downsrob@amazon.com> * Update maven publication to include cksums. (#224) This change adds a task to publish to a local staging repo under build/ that includes cksums. It also updates build.sh to use this new task and copy the contents of the staging repo to the output directory. The maven publish plugin will not include these cksums when publishing to maven local but will when published to a separate folder. Signed-off-by: Marc Handalian <handalm@amazon.com> * Add release notes for 1.2.0.0 release (#225) * Create opensearch-alerting.release-notes-1.2.0.0.md Signed-off-by: Annie Lee <leeyun@amazon.com> * Update opensearch-alerting.release-notes-1.2.0.0.md * Update opensearch-alerting.release-notes-1.2.0.0.md * Add backwards compatibility tests (#199) * Initial commit for BWC tests Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Update bwc test to check Monitor stats and add bwc tests to GitHub Actions Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Use current version plugin bundle from build for bwc tests instead of manually uploading Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Update mockito-core dependency to 3.12.4 to prevent conflict Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Remove disabling security manager flag when running BWC tests Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> Co-authored-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> Co-authored-by: Sriram <59816283+skkosuri-amzn@users.noreply.github.com> Co-authored-by: Clay Downs <89109232+downsrob@users.noreply.github.com> Co-authored-by: Abbas Hussain <abbashus@amazon.com> Co-authored-by: Daniel Doubrovkine (dB.) <dblock@dblock.org> Co-authored-by: Marc Handalian <handalm@amazon.com> Co-authored-by: Annie Lee <71157062+leeyun-amzn@users.noreply.github.com>

* Update copyright notice (opensearch-project#222) Signed-off-by: Mohammad Qureshi <qreshi@amazon.com> * Admin Users must be able to access all monitors opensearch-project#139 (opensearch-project#220) * Admin Users must be able to access all monitors opensearch-project#139 * Refactored Co-authored-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> Co-authored-by: Sriram <59816283+skkosuri-amzn@users.noreply.github.com> Signed-off-by: AWSHurneyt <hurneyt@amazon.com>

Admin Users must be able to access all monitors opensearch-project#139

2f19583

skkosuri-amzn requested review from lezzago and qreshi October 29, 2021 20:48

Refactored

031d9fc

qreshi reviewed Nov 1, 2021

View reviewed changes

lezzago approved these changes Nov 1, 2021

View reviewed changes

qreshi approved these changes Nov 2, 2021

View reviewed changes

skkosuri-amzn merged commit 8b337e7 into opensearch-project:main Nov 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Admin Users must be able to access all monitors #139#220

Admin Users must be able to access all monitors #139#220
skkosuri-amzn merged 2 commits intoopensearch-project:mainfrom
skkosuri-amzn:admin_all_access2

skkosuri-amzn commented Oct 29, 2021 •

edited

Loading

Uh oh!

codecov-commenter commented Oct 29, 2021 •

edited

Loading

Uh oh!

Uh oh!

qreshi Nov 1, 2021

Uh oh!

lezzago Nov 1, 2021

Uh oh!

skkosuri-amzn Nov 1, 2021

Uh oh!

qreshi Nov 2, 2021

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

skkosuri-amzn commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

qreshi Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

lezzago Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

skkosuri-amzn Nov 1, 2021

Choose a reason for hiding this comment

Uh oh!

qreshi Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

skkosuri-amzn commented Oct 29, 2021 •

edited

Loading

codecov-commenter commented Oct 29, 2021 •

edited

Loading