Add logged-on User details to the Monitor and Destination #255

skkosuri-amzn · 2020-09-23T20:57:51Z

Issue #, if available:
#6
#215

Description of changes:
Add logged-on User details to the Monitor and Destination
This one of PR's for adding security to alerting.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…tination response, 3/ handle null in GetMonitorRequest

…rch/alerting

codecov · 2020-09-23T21:45:54Z

Codecov Report

Merging #255 into master will increase coverage by 0.21%.
The diff coverage is 79.38%.

@@             Coverage Diff              @@
##             master     #255      +/-   ##
============================================
+ Coverage     77.31%   77.53%   +0.21%     
- Complexity      187      198      +11     
============================================
  Files           137      138       +1     
  Lines          4466     4522      +56     
  Branches        594      614      +20     
============================================
+ Hits           3453     3506      +53     
+ Misses          701      691      -10     
- Partials        312      325      +13

Impacted Files	Coverage Δ	Complexity Δ
...relasticsearch/alerting/core/model/ScheduledJob.kt	`86.66% <ø> (ø)`	`0.00 <0.00> (ø)`
...ticsearch/alerting/elasticapi/ElasticExtensions.kt	`56.81% <33.33%> (-1.72%)`	`0.00 <0.00> (ø)`
...endistroforelasticsearch/alerting/model/Monitor.kt	`88.70% <71.42%> (-2.52%)`	`0.00 <0.00> (ø)`
...distroforelasticsearch/alerting/core/model/User.kt	`81.39% <81.39%> (ø)`	`6.00 <6.00> (?)`
...icsearch/alerting/model/destination/Destination.kt	`62.93% <85.71%> (-2.32%)`	`0.00 <0.00> (ø)`
...elasticsearch/alerting/action/GetMonitorRequest.kt	`100.00% <100.00%> (+4.00%)`	`0.00 <0.00> (ø)`
...e/action/node/ScheduledJobsStatsTransportAction.kt	`75.00% <0.00%> (ø)`	`8.00% <0.00%> (ø%)`
...asticsearch/alerting/core/schedule/JobScheduler.kt	`75.71% <0.00%> (+4.28%)`	`15.00% <0.00%> (ø%)`
...rch/alerting/core/action/node/ScheduledJobStats.kt	`66.66% <0.00%> (+11.11%)`	`4.00% <0.00%> (+1.00%)`
... and 1 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 48ba52a...8e1092f. Read the comment docs.

dbbaughe · 2020-09-23T21:34:39Z

...g/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/action/GetMonitorRequest.kt

        if (srcContext != null) {
            out.writeBoolean(true)
-            srcContext?.writeTo(out)
+            srcContext.writeTo(out)
        } else {
            out.writeBoolean(false)
        }


nit: Maybe cleaner?

out.writeBoolean(srcContext != null) srcContext?.writeTo(out)

like it 👍

dbbaughe · 2020-09-23T21:36:46Z

alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/model/Monitor.kt

@@ -117,8 +122,15 @@ data class Monitor(

    override fun fromDocument(id: String, version: Long): Monitor = copy(id = id, version = version)

+    private fun XContentBuilder.optionalUserField(user: User?): XContentBuilder {


nit: Move to ElasticExtensions

dbbaughe · 2020-09-23T21:37:25Z

alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/model/Monitor.kt

@@ -131,6 +143,12 @@ data class Monitor(
        schedule.writeTo(out)
        out.writeInstant(lastUpdateTime)
        out.writeOptionalInstant(enabledTime)
+        if (user != null) {


nit: Same as above

out.writeBoolean(user != null) user?.writeTo(out)

dbbaughe · 2020-09-23T21:38:12Z

alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/model/User.kt

@@ -0,0 +1,92 @@
+package com.amazon.opendistroforelasticsearch.alerting.model


Missing License

dbbaughe · 2020-09-23T21:40:30Z

alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/model/User.kt

+
+    @Throws(IOException::class)
+    constructor(sin: StreamInput): this(
+            sin.readString(), // name


nit: I prefer using the named arguments instead of comments
i.e.

@Throws(IOException::class) constructor(sin: StreamInput): this( name = sin.readString(), backendRoles = sin.readStringList(), roles = sin.readStringList(), customAttNames = sin.readStringList() )

+1. Still kotlin newbie

dbbaughe · 2020-09-23T21:45:52Z

alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/model/User.kt

+                    }
+                }
+            }
+            return User(name, backendRoles, roles, customAttNames)


Do we want any validation of the values here? Or is a User("", listOf(), listOf(), listOf()) a valid user?

dbbaughe · 2020-09-23T21:46:52Z

alerting/src/test/kotlin/com/amazon/opendistroforelasticsearch/alerting/TestHelpers.kt

@@ -164,6 +182,14 @@ fun Monitor.toJsonString(): String {
    return this.toXContent(builder).string()
 }

+fun randomUser(): User {
+    return User("joe", listOf("ops", "backup"), listOf("all_access"), listOf("test_attr=test"))


These don't really seem random 😛

dbbaughe · 2020-09-23T21:49:55Z

alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/model/User.kt

+import org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken
+import java.io.IOException
+
+data class User(


I was under the impression we wanted to decouple the roles from the user when storing this on the Monitor?

Having name, roles, backend_roles as user (sub) object is much clean. Checks are simpler. monitor.user

dbbaughe · 2020-09-23T21:51:53Z

core/src/main/resources/mappings/scheduled-jobs.json

+              "type" : "text",
+              "fields" : {
+                "keyword" : {
+                  "type" : "text"


Shouldn't this be type keyword? Same with below.

Good catch. Yes it has to be keyword. Merged old version.

dbbaughe · 2020-09-23T21:52:07Z

core/src/main/resources/mappings/scheduled-jobs.json

+              "type" : "text",
+              "fields" : {
+                "keyword" : {
+                  "type" : "text"


Same comment as below.

…rch/alerting

dbbaughe · 2020-09-25T16:46:41Z

alerting/src/test/kotlin/com/amazon/opendistroforelasticsearch/alerting/TestHelpers.kt

+    val ranStrGen = RandomStringGenerator.Builder().build()
+    val randomUser = ranStrGen.generate(5)
+    val bckEndRole1 = ranStrGen.generate(10)
+    val bckEndRole2 = ranStrGen.generate(10)


There are already provided helper methods for generating random values, e.g. ESRestTestCase.randomAlphaOfLength(10)

saratvemulapalli · 2020-09-25T17:16:45Z

...g/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/action/GetMonitorRequest.kt

-        } else {
-            out.writeBoolean(false)
-        }
+        out.writeBoolean(srcContext != null)


This is nice!

saratvemulapalli

Changes look good to me.

skkosuri-amzn added 3 commits September 9, 2020 15:49

Fix 1/ IllegalStateException warn messages, 2/ location header in des…

cffa376

…tination response, 3/ handle null in GetMonitorRequest

Merge branch 'master' of https://github.com/opendistro-for-elasticsea…

1ceb3a9

…rch/alerting

Add User details to Monitor and Destination

ff14293

skkosuri-amzn requested review from lezzago, saratvemulapalli, dbbaughe, qreshi, ftianli-amzn and bowenlan-amzn September 23, 2020 20:57

skkosuri-amzn self-assigned this Sep 23, 2020

dbbaughe reviewed Sep 23, 2020

View reviewed changes

skkosuri-amzn added 4 commits September 24, 2020 23:36

refacted

39411a0

Merge branch 'master' of https://github.com/opendistro-for-elasticsea…

58baa44

…rch/alerting

Merged from master

c37ed20

Increase schema version

c8c476c

dbbaughe reviewed Sep 25, 2020

View reviewed changes

lezzago previously approved these changes Sep 25, 2020

View reviewed changes

saratvemulapalli reviewed Sep 25, 2020

View reviewed changes

saratvemulapalli previously approved these changes Sep 25, 2020

View reviewed changes

Use ESRestTestCase.randomAlphaOfLength for random str

8e1092f

skkosuri-amzn dismissed stale reviews from saratvemulapalli and lezzago via 8e1092f September 25, 2020 21:06

lezzago approved these changes Sep 25, 2020

View reviewed changes

saratvemulapalli approved these changes Sep 25, 2020

View reviewed changes

skkosuri-amzn merged commit 9697682 into master Sep 25, 2020

qreshi deleted the add-user-pr branch December 19, 2020 00:45

tlfeng pushed a commit that referenced this pull request Feb 6, 2021

Add logged-on User details to the Monitor and Destination (#255)

5222a18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logged-on User details to the Monitor and Destination #255

Add logged-on User details to the Monitor and Destination #255

skkosuri-amzn commented Sep 23, 2020

codecov bot commented Sep 23, 2020 •

edited

Loading

dbbaughe Sep 23, 2020

skkosuri-amzn Sep 24, 2020

dbbaughe Sep 23, 2020

skkosuri-amzn Sep 25, 2020

dbbaughe Sep 23, 2020

dbbaughe Sep 23, 2020

dbbaughe Sep 23, 2020

skkosuri-amzn Sep 25, 2020

dbbaughe Sep 23, 2020

dbbaughe Sep 23, 2020

skkosuri-amzn Sep 25, 2020

dbbaughe Sep 23, 2020

skkosuri-amzn Sep 25, 2020

dbbaughe Sep 23, 2020

skkosuri-amzn Sep 25, 2020

dbbaughe Sep 23, 2020

dbbaughe Sep 25, 2020

saratvemulapalli Sep 25, 2020

saratvemulapalli left a comment

		@@ -117,8 +122,15 @@ data class Monitor(

		override fun fromDocument(id: String, version: Long): Monitor = copy(id = id, version = version)

		private fun XContentBuilder.optionalUserField(user: User?): XContentBuilder {

		@@ -0,0 +1,92 @@
		package com.amazon.opendistroforelasticsearch.alerting.model

Add logged-on User details to the Monitor and Destination #255

Add logged-on User details to the Monitor and Destination #255

Conversation

skkosuri-amzn commented Sep 23, 2020

codecov bot commented Sep 23, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saratvemulapalli left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 23, 2020 •

edited

Loading