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

[Security Solutions][Detection Engine] Fixes "undefined" crash for author field by adding a migration for it #107230

Merged
merged 6 commits into from Aug 11, 2021

Conversation

FrankHassanabad
Copy link
Contributor

@FrankHassanabad FrankHassanabad commented Jul 29, 2021

Summary

Fixes #106233

During an earlier upgrade/fix to our system to add defaults to our types, we overlooked the "author" field which wasn't part of the original rules. Users upgrading might get errors such as:

params invalid: Invalid value "undefined" supplied to "author"

This fixes that issue by adding a migration for the author field for 7.14.1.

See #106233 for test instructions or manually remove your author field before upgrading your release and then upgrade and this should be fixed on upgrade.

Checklist

@FrankHassanabad FrankHassanabad self-assigned this Jul 29, 2021
@FrankHassanabad FrankHassanabad added v8.0.0 v7.15.0 v7.14.1 Feature:Detection Rules Anything related to Security Solution's Detection Rules impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. release_note:fix labels Jul 29, 2021
@FrankHassanabad FrankHassanabad marked this pull request as ready for review July 29, 2021 19:19
@FrankHassanabad FrankHassanabad requested a review from a team as a code owner July 29, 2021 19:19
@mikecote mikecote added this to In Review in Kibana Alerting Jul 29, 2021
Copy link
Contributor

@marshallmain marshallmain left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for catching and fixing this!

@@ -970,6 +970,26 @@ describe('successful migrations', () => {
});
});
});

describe('7.14.1', () => {
test('security solution author field is migrated to array', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a test to make sure existing author fields aren't overridden?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh thank you, I will add that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added here: f507b65

...doc.attributes,
params: {
...params,
author: params.author != null ? params.author : [],
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be possible for the author field to be non-null and not an array? just a string maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, when we introduced it, it was always an array of values as seen here: https://github.com/elastic/kibana/pull/70288/files

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM

@FrankHassanabad FrankHassanabad enabled auto-merge (squash) August 10, 2021 22:00
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @FrankHassanabad

@FrankHassanabad FrankHassanabad merged commit 978c44e into elastic:master Aug 11, 2021
Kibana Alerting automation moved this from In Review to Done (Ordered by most recent) Aug 11, 2021
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 11, 2021
…-png-pdf-report-type

* 'master' of github.com:elastic/kibana: (101 commits)
  [ML] APM Latency Correlations: Field/value candidates prioritization (elastic#107370)
  [Reporting] Add lenience to a test on the order of asserted logs (elastic#108135)
  [Lens] fix do not submit invalid query in filtered metric (elastic#107542)
  skip flaky test (elastic#108043)
  fix newly introduced type error (elastic#107593)
  [Reporting] server side code clean up (elastic#106940)
  [build_ts_refs] improve caches, allow building a subset of projects (elastic#107981)
  [APM] Add new ftr_e2e to kibana CI and remove current e2e tests. (elastic#107593)
  add manage rules link to alerts dropdown (elastic#107950)
  [ML] Enable Index data visualizer document count chart to update time range query (elastic#106438)
  [Security Solutions][Detection Engine] Fixes "undefined" crash for author field by adding a migration for it (elastic#107230)
  [Actions UI] Fixed Jira Api token label. (elastic#107776)
  [Alerting UI] Fixed display permissions for edit/delete buttons when user has read only access. (elastic#107996)
  [Maps] fix code owners (elastic#108106)
  Update EMS landing page url (elastic#108102)
  Do not render page header for loading domains (elastic#108078)
  Update dependency @elastic/charts to v33.2.2 (elastic#107939)
  [APM] Display throughput as tps (instead of tpm) when bucket size < 60 seconds (elastic#107850)
  [Fleet] Fix all category count (elastic#108089)
  [Security Solution][Bug] - Disable alert table RBAC until fields sorted (elastic#108034)
  ...

# Conflicts:
#	x-pack/plugins/reporting/server/export_types/common/generate_png.ts
#	x-pack/plugins/reporting/server/lib/screenshots/index.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.ts
@FrankHassanabad FrankHassanabad added auto-backport Deprecated: Automatically backport this PR after it's merged and removed auto-backport Deprecated: Automatically backport this PR after it's merged labels Aug 11, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 11, 2021
…thor field by adding a migration for it (elastic#107230)

## Summary

Fixes elastic#106233

During an earlier upgrade/fix to our system to add defaults to our types, we overlooked the "author" field which wasn't part of the original rules. Users upgrading might get errors such as:

```
params invalid: Invalid value "undefined" supplied to "author"
```

This fixes that issue by adding a migration for the `author` field for `7.14.1`.

See elastic#106233 for test instructions or manually remove your author field before upgrading your release and then upgrade and this should be fixed on upgrade.


### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.x
7.14 Commit could not be cherrypicked due to conflicts

Successful backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 107230

FrankHassanabad added a commit to FrankHassanabad/kibana that referenced this pull request Aug 11, 2021
…thor field by adding a migration for it (elastic#107230)

## Summary

Fixes elastic#106233

During an earlier upgrade/fix to our system to add defaults to our types, we overlooked the "author" field which wasn't part of the original rules. Users upgrading might get errors such as:

```
params invalid: Invalid value "undefined" supplied to "author"
```

This fixes that issue by adding a migration for the `author` field for `7.14.1`.

See elastic#106233 for test instructions or manually remove your author field before upgrading your release and then upgrade and this should be fixed on upgrade.


### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
FrankHassanabad added a commit to FrankHassanabad/kibana that referenced this pull request Aug 11, 2021
…thor field by adding a migration for it (elastic#107230)

## Summary

Fixes elastic#106233

During an earlier upgrade/fix to our system to add defaults to our types, we overlooked the "author" field which wasn't part of the original rules. Users upgrading might get errors such as:

```
params invalid: Invalid value "undefined" supplied to "author"
```

This fixes that issue by adding a migration for the `author` field for `7.14.1`.

See elastic#106233 for test instructions or manually remove your author field before upgrading your release and then upgrade and this should be fixed on upgrade.

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

# Conflicts:
#	x-pack/plugins/alerting/server/saved_objects/migrations.test.ts
kibanamachine added a commit that referenced this pull request Aug 11, 2021
…thor field by adding a migration for it (#107230) (#108218)

## Summary

Fixes #106233

During an earlier upgrade/fix to our system to add defaults to our types, we overlooked the "author" field which wasn't part of the original rules. Users upgrading might get errors such as:

```
params invalid: Invalid value "undefined" supplied to "author"
```

This fixes that issue by adding a migration for the `author` field for `7.14.1`.

See #106233 for test instructions or manually remove your author field before upgrading your release and then upgrade and this should be fixed on upgrade.


### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Co-authored-by: Frank Hassanabad <frank.hassanabad@elastic.co>
FrankHassanabad added a commit that referenced this pull request Aug 11, 2021
… for author field by adding a migration for it (#107230) (#108221)

* [Security Solutions][Detection Engine] Fixes "undefined" crash for author field by adding a migration for it (#107230)

## Summary

Fixes #106233

During an earlier upgrade/fix to our system to add defaults to our types, we overlooked the "author" field which wasn't part of the original rules. Users upgrading might get errors such as:

```
params invalid: Invalid value "undefined" supplied to "author"
```

This fixes that issue by adding a migration for the `author` field for `7.14.1`.

See #106233 for test instructions or manually remove your author field before upgrading your release and then upgrade and this should be fixed on upgrade.

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

# Conflicts:
#	x-pack/plugins/alerting/server/saved_objects/migrations.test.ts

* Fixed merge mistakes as the two test versions are very different

* Fixes another area where the backport system is very different
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Detection Rules Anything related to Security Solution's Detection Rules impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. release_note:fix v7.14.1 v7.15.0 v8.0.0
Projects
No open projects
Kibana Alerting
Done (Ordered by most recent)
Development

Successfully merging this pull request may close these issues.

[Security Solution]An error "Rule failure" is getting displayed on Rule detail page.
6 participants