-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Watcher: Store username on watch execution #31873
Conversation
There is currently no way to see what user executed a watch. This commit adds the decrypted username to each execution in the watch history, in a new field executed_by. Closes elastic#31772
Pinging @elastic/es-core-infra |
One question here... should this also be backported to 6.3? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left one major comment regarding naming and asked for a unit test, but almost there. In general I think this can go into 6.4 and master branches
@@ -43,12 +43,14 @@ | |||
private static final ParseField METADATA = new ParseField("metadata"); | |||
private static final ParseField EXECUTION_RESULT = new ParseField("result"); | |||
private static final ParseField EXCEPTION = new ParseField("exception"); | |||
private static final ParseField EXECUTED_BY = new ParseField("executed_by"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about simply naming this user
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one simple change to affect the whole PR ;)
@@ -85,6 +88,14 @@ public Watch watch() { | |||
public void ensureWatchExists(CheckedSupplier<Watch, Exception> supplier) throws Exception { | |||
if (watch == null) { | |||
watch = supplier.get(); | |||
// now that the watch exists, extract out the authentication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be refactored into a small static method, so you could write also some tests (so many ifs/null checks to check). This could also be done lazily in the getter (not sure yet if that is a good idea though).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method could also be overwritten by sub classes (both happen to call super.ensureExists()
though) - might be a good reason to load it lazily
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There looks to be no need to override this method, so ill make it final (and still move that bit to a static helper for testing)
edit: disregard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after some snooping I noticed we can and should do this #31926
id: "my_watch" | ||
- match: { watch_record.watch_id: "my_watch" } | ||
- match: { watch_record.state: "executed" } | ||
- match: { watch_record.executed_by: "x_pack_rest_user" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Addressed comments and created a test for the Watch Execution Context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the doc tests are failing due to the newly added field, and need to be fixed first. codewise I think we are good.
assertNull(context.getUser()); | ||
|
||
Map<String, String> headers = new HashMap<>(); | ||
headers.put(AuthenticationField.AUTHENTICATION_KEY, encoded); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use Collections.singletonMap()
here for convenience. Can you move the Authentication
code down here, where the map gets created?
@@ -179,6 +183,9 @@ public final XContentBuilder toXContent(XContentBuilder builder, Params params) | |||
if (executionResult != null) { | |||
builder.field(EXECUTION_RESULT.getPreferredName(), executionResult, params); | |||
} | |||
if (user != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you maybe move this up to where the node id gets written out? This way, one can see both at once when reading a watch history, otherwise one is at the top and the other at the bottom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed, will do this in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nm, i can do it on this one! I thought i had merged already woop woop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spinscale can you eyeball 0dc862f just so im sure i did what u asked?
@@ -152,6 +156,9 @@ public final XContentBuilder toXContent(XContentBuilder builder, Params params) | |||
builder.field(NODE.getPreferredName(), nodeId); | |||
builder.field(STATE.getPreferredName(), state.id()); | |||
|
|||
if (user != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
* es/master: (21 commits) Tweaked Elasticsearch Service links for SEO Watcher: Store username on watch execution (#31873) Use correct formatting for links (#29460) Painless: Separate PainlessLookup into PainlessLookup and PainlessLookupBuilder (#32054) Scripting: Remove dead code from painless module (#32064) [Rollup] Replace RollupIT with a ESRestTestCase version (#31977) [TEST] Consistent algorithm usage (#32077) [Rollup] Fix duplicate field names in test (#32075) Ensure only parent breaker trips in unit test Unmute field collapsing rest tests Fix BWC check after backport [Tests] Fix failure due to changes exception message (#32036) Remove unused params from SSource and Walker (#31935) [Test] Mute MlJobIT#testDeleteJobAfterMissingAliases Turn off real-mem breaker in REST tests Turn off real-mem breaker in single node tests Fix broken OpenLDAP Vagrant QA test Cleanup Duplication in `PainlessScriptEngine` (#31991) SCRIPTING: Remove unused MultiSearchTemplateRequestBuilder (#32049) Fix compile issues introduced by merge (#32058) ...
There is currently no way to see what user executed a watch. This commit adds the decrypted username to each execution in the watch history, in a new field "user". Closes #31772
* 6.x: Security: revert to old way of merging automata (#32254) Fix a test bug in RangeQueryBuilderTests introduced in the field aliases backport. Introduce Application Privileges with support for Kibana RBAC (#32309) Undo a debugging change that snuck in during the field aliases merge. [test] port linux package packaging tests (#31943) Painless: Update More Methods to New Naming Scheme (#32305) Tribe: Add error with secure settings copied to tribe (#32298) Add V_6_3_3 version constant Add ERR to ranking evaluation documentation (#32314) [DOCS] Added link to 6.3.2 RNs [DOCS] Updates 6.3.2 release notes with PRs from ml-cpp repo (#32334) [Kerberos] Add Kerberos authentication support (#32263) [ML] Extract persistent task methods from MlMetadata (#32319) Backport - Add Snapshots Status API to High Level Rest Client (#32295) Make release notes ignore the `>test-failure` label. (#31309) [DOCS] Adds release highlights for search for 6.4 (#32095) Allow Integ Tests to run in a FIPS-140 JVM (#32316) Add support for field aliases to 6.x. (#32184) Register ERR metric with NamedXContentRegistry (#32320) fixes broken build for third-party-tests (#32315) Relates #31918 / Closes infra/issues/6085 [DOCS] Rollup Caps API incorrectly mentions GET Jobs API (#32280) Rest HL client: Add put watch action (#32026) (#32191) Add WeightedAvg metric aggregation (#31037) Consistent encoder names (#29492) Switch monitoring to new style Requests (#32255) specify subdirs of lib, bin, modules in package (#32253) Rename ranking evaluation `quality_level` to `metric_score` (#32168) Add new permission for JDK11 to load JAAS libraries (#32132) Switch x-pack:core to new style Requests (#32252) Watcher: Store username on watch execution (#31873) Silence SSL reload test that fails on JDK 11 Painless: Clean up add methods in PainlessLookup (#32258) CCE when re-throwing "shard not available" exception in TransportShardMultiGetAction (#32185) Fail shard if IndexShard#storeStats runs into an IOException (#32241) Fix `range` queries on `_type` field for singe type indices (#31756) (#32161) AwaitsFix RecoveryIT#testHistoryUUIDIsGenerated Add new fields to monitoring template for Beats state (#32085) (#32273) [TEST] improve REST high-level client naming conventions check (#32244) Check that client methods match API defined in the REST spec (#31825)
The tests we referring to an older watch history template, that was updated as part of b982e1a (initial PR was elastic#31873) Closes elastic#32307
as per elastic/elasticsearch#31873 (cherry picked from commit 7f99a91)
There is currently no way to see what user executed a watch. This commit
adds the decrypted username to each execution in the watch history, in a
new field executed_by.
Closes #31772