Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug fix for: Admin Users must be able to access all monitors #139 #280

Merged
merged 1 commit into from
Jan 15, 2022

Conversation

skkosuri-amzn
Copy link
Contributor

Issue #, if available:
Bug fix for: Admin Users must be able to access all monitors #139

Description of changes:
Fix GET /monitor DELETE /monitor when
plugins.alerting.filter_by_backend_roles : true

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.

@skkosuri-amzn skkosuri-amzn requested review from a team, adityaj1107, lezzago and qreshi January 14, 2022 22:18
emptyMap(),
NStringEntity(search, ContentType.APPLICATION_JSON)
)
assertEquals("Get monitor failed", RestStatus.OK, adminGetResponse.restStatus())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a more descriptive error message?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is fine since its is part of the assertion failure which would mean the request failed to get the monitor

@@ -509,6 +509,25 @@ class SecureMonitorRestApiIT : AlertingRestTestCase() {
)
assertEquals("Search monitor failed", RestStatus.OK, adminSearchResponse.restStatus())
assertEquals("Monitor not found during search", 1, getDocs(adminSearchResponse))

// get as "admin" - must get 1 docs
val id: String = monitorJson["_id"] as String
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add assertion check for ID?

@codecov-commenter
Copy link

Codecov Report

Merging #280 (cb6f449) into main (f92dec0) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #280   +/-   ##
=========================================
  Coverage     78.63%   78.63%           
+ Complexity      218      217    -1     
=========================================
  Files           173      173           
  Lines          6968     6970    +2     
  Branches        915      916    +1     
=========================================
+ Hits           5479     5481    +2     
+ Misses         1002     1001    -1     
- Partials        487      488    +1     
Impacted Files Coverage Δ
...search/alerting/transport/SecureTransportAction.kt 39.02% <0.00%> (+8.25%) ⬆️
...ing/destination/client/DestinationEmailClient.java 72.50% <0.00%> (-5.00%) ⬇️
...in/kotlin/org/opensearch/alerting/MonitorRunner.kt 71.38% <0.00%> (-0.68%) ⬇️
.../kotlin/org/opensearch/alerting/core/JobSweeper.kt 71.50% <0.00%> (-0.50%) ⬇️
...ing/model/destination/DestinationContextFactory.kt 66.66% <0.00%> (ø)
...ain/kotlin/org/opensearch/alerting/AlertService.kt 79.02% <0.00%> (+0.48%) ⬆️
...lin/org/opensearch/alerting/alerts/AlertIndices.kt 67.11% <0.00%> (+1.34%) ⬆️

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 9113c04...cb6f449. Read the comment docs.

@skkosuri-amzn skkosuri-amzn merged commit 0351155 into opensearch-project:main Jan 15, 2022
lezzago pushed a commit to lezzago/alerting-opensearch that referenced this pull request Mar 9, 2022
lezzago pushed a commit to lezzago/alerting-opensearch that referenced this pull request Mar 9, 2022
…rch-project#139 (opensearch-project#280)

Signed-off-by: Sriram <59816283+skkosuri-amzn@users.noreply.github.com>
lezzago pushed a commit to lezzago/alerting-opensearch that referenced this pull request Mar 9, 2022
…rch-project#139 (opensearch-project#280)

Signed-off-by: Sriram <59816283+skkosuri-amzn@users.noreply.github.com>
lezzago pushed a commit that referenced this pull request Mar 9, 2022
)

Signed-off-by: Sriram <59816283+skkosuri-amzn@users.noreply.github.com>
AWSHurneyt pushed a commit to AWSHurneyt/OpenSearch-Alerting that referenced this pull request Mar 30, 2022
…rch-project#139 (opensearch-project#280)

Signed-off-by: Sriram <59816283+skkosuri-amzn@users.noreply.github.com>
Signed-off-by: AWSHurneyt <hurneyt@amazon.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 23, 2022
)

Signed-off-by: skkosuri-amzn <skkosuri@amazon.com>
(cherry picked from commit 0351155)
qreshi pushed a commit that referenced this pull request Sep 23, 2022
)

Signed-off-by: Sriram <59816283+skkosuri-amzn@users.noreply.github.com>
(cherry picked from commit 0351155)
qreshi pushed a commit that referenced this pull request Sep 23, 2022
) (#565)

Signed-off-by: Sriram <59816283+skkosuri-amzn@users.noreply.github.com>
(cherry picked from commit 0351155)

Co-authored-by: Sriram <59816283+skkosuri-amzn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants