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][Detections] - Fix bug, last response not showing for disabled rules #81783

Merged
merged 5 commits into from
Oct 28, 2020

Conversation

yctercero
Copy link
Contributor

@yctercero yctercero commented Oct 27, 2020

Summary

Bug fix addressed in this PR:

Example of bug

78947738-e046ca80-7a8b-11ea-855a-b6e4a5cd9073

Example of fix

rule_disable

Checklist

@yctercero yctercero requested review from a team as code owners October 27, 2020 14:26
@yctercero yctercero self-assigned this Oct 27, 2020
@yctercero yctercero added Feature:Detection Rules Anything related to Security Solution's Detection Rules release_note:fix Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.11.0 v8.0.0 labels Oct 27, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@@ -74,7 +75,7 @@ export const transformValidateBulkError = (
ruleId: string,
alert: PartialAlert,
ruleActions?: RuleActions | null,
ruleStatus?: unknown
ruleStatus?: SavedObjectsFindResponse<IRuleStatusSOAttributes>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't typed because some areas were passing through ruleStatuses.saved_objects[0] and some were passing ruleStatuses. Added the correct type and adjusted use cases so they all just pass in ruleStatuses since we were already making checks within for the existence of .saved_objects[0] .

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think the pattern from Kibana core/other areas go:

  • They don't have types or use any or unknown, so let's use unknown for now/cast.
  • Later someone adds templates, types, such as SavedObjectsFindResponse
  • Someone else looks at our code and says, "lol, why you using unknown here, let me change that"

So thank you very much. This looks better.

@yctercero
Copy link
Contributor Author

@elasticmachine merge upstream

},
rule_id: 'rule-1',
};
expect(validatedOrError).toEqual(expected);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Through out this test I would recommend just weakening your types to avoid the ts-expect-error (but I do appreciate you using ts-expect-error 👍 like so:

    test('it should return error object if "alert" is not expected alert type', () => {
      const { alertTypeId, ...ruleAlert } = getResult();
      const validatedOrError = transformValidateBulkError('rule-1', ruleAlert);
      const expected: BulkError = {
        error: {
          message: 'Internal error transforming',
          status_code: 500,
        },
        rule_id: 'rule-1',
      };
      expect(validatedOrError).toEqual(expected);
    });

Either way works, I mostly use this way to avoid the newer TS errors on deleting things that used to not be there.

Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

LGTM! Love all of it, makes everything look so much more useful and better.

@yctercero
Copy link
Contributor Author

@elasticmachine merge upstream

@yctercero
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
securitySolution 8.1MB 8.1MB +95.0B

History

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

@yctercero yctercero merged commit 2788dca into elastic:master Oct 28, 2020
yctercero added a commit to yctercero/kibana that referenced this pull request Oct 28, 2020
… for disabled rules (elastic#81783)

## Summary

**Bug fix addressed in this PR:**
- fixes elastic#63203
- in the backend, we were only fetching status data for enabled rules, changed to fetch status data regardles of whether rule is enabled or disabled
yctercero added a commit that referenced this pull request Oct 28, 2020
… for disabled rules (#81783) (#81867)

## Summary

**Bug fix addressed in this PR:**
- fixes #63203
- in the backend, we were only fetching status data for enabled rules, changed to fetch status data regardles of whether rule is enabled or disabled
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 28, 2020
…kibana into task-manager/lost-connectivity

* 'task-manager/lost-connectivity' of github.com:gmmorris/kibana:
  skips overview tests (elastic#81877)
  [Security Solution][Case] Fix connector's labeling (elastic#81824)
  [Maps] Fix EMS test (elastic#81856)
  [Security Solutions][Detections] - Fix bug, last response not showing for disabled rules (elastic#81783)
  skip flaky suite (elastic#81853)
  Add tsconfig for url_forwarding (elastic#81177)
  skip flaky suite (elastic#81844)
  check for server enabled (elastic#81818)
  [Seurity Solution][Case] Create case plugin client (elastic#81018)
  [Security Solutions][Detection Engine] Changes wording for threat matches and rules (elastic#81334)
  [Security Solution] critical pref bug with browser fields reducer
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 29, 2020
…kibana into alerts/convert-to-tm-intervals

* 'alerts/convert-to-tm-intervals' of github.com:gmmorris/kibana: (88 commits)
  fixed jest
  APM Experiments settings (elastic#81554)
  [Resolver] Enable resolver test plugin tests (elastic#81339)
  Add TS project references for inspector  (elastic#81792)
  Add uri decode to es_ui_shared and fix navigation issues with special characters (elastic#80835)
  [Fleet] Rename ingestManager translations fleet (elastic#81837)
  [Logs UI] Transmit and render array field values in log entries (elastic#81385)
  Audit Logging: use the original url (elastic#81282)
  [User experience] Fix JS error rate (elastic#81512)
  [UX] Add median/percentile info in titles (elastic#79824)
  Support export for SO with circular refs (elastic#81582)
  Get rid of  global types (elastic#81739)
  [APM] Fix precommit script (elastic#81594)
  skips overview tests (elastic#81877)
  [Security Solution][Case] Fix connector's labeling (elastic#81824)
  Added simple test, which only covers successful case when edit happened right after task was complete previous execution
  [Maps] Fix EMS test (elastic#81856)
  [Security Solutions][Detections] - Fix bug, last response not showing for disabled rules (elastic#81783)
  skip flaky suite (elastic#81853)
  Fixed type checks and unit tests
  ...
@yctercero yctercero deleted the last_run_bug branch December 6, 2020 19:20
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 release_note:fix Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SIEM] Last run/response are not displayed for a disabled rule
4 participants