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.

Sign up for GitHub

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

Jump to bottom

[Security Solutions] Adds a default for indicator match custom query of *:* #81727

Merged
merged 14 commits into from
Nov 12, 2020
Merged

[Security Solutions] Adds a default for indicator match custom query of *:* #81727

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4dc7f3c
Adds default for threat matching query
FrankHassanabad Oct 27, 2020
4f7756c
Merge branch 'master' into add-default-for-threat-match
FrankHassanabad Oct 27, 2020
9f98bd6
Merge branch 'master' into add-default-for-threat-match
FrankHassanabad Nov 3, 2020
eb32912
Merge branch 'master' into add-default-for-threat-match
FrankHassanabad Nov 6, 2020
a998547
Merge branch 'master' into add-default-for-threat-match
FrankHassanabad Nov 6, 2020
6ac90aa
Fix bugs found during PR review
FrankHassanabad Nov 6, 2020
7bbc2a1
Merge branch 'master' into add-default-for-threat-match
kibanamachine Nov 9, 2020
805ce94
Updates to reduce state and use different technique with references f…
FrankHassanabad Nov 11, 2020
402348f
Merge branch 'add-default-for-threat-match' of github.com:FrankHassan…
FrankHassanabad Nov 11, 2020
8dee519
Merge branch 'master' into add-default-for-threat-match
FrankHassanabad Nov 11, 2020
0f234ef
Fixed formatting to not show a change
FrankHassanabad Nov 11, 2020
499e721
Reverted other changes to reduce the diff on the commit to even small…
FrankHassanabad Nov 11, 2020
abdaa53
Removes unneeded change
FrankHassanabad Nov 11, 2020
ed94de2
Updates docs a bit
FrankHassanabad Nov 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...security_solution/public/detections/components/rules/eql_query_bar/eql_query_bar.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ describe('EqlQueryBar', () => {

const expected = {
filters: [],
edited: true,
query: {
query: 'newQuery',
language: 'eql',
Expand Down
1 change: 1 addition & 0 deletions ...gins/security_solution/public/detections/components/rules/eql_query_bar/eql_query_bar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ export const EqlQueryBar: FC<EqlQueryBarProps> = ({
query: newQuery,
language: 'eql',
},
edited: true,
});
},
[setValue]
Expand Down
9 changes: 7 additions & 2 deletions x-pack/plugins/security_solution/public/detections/components/rules/query_bar/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ export const QueryBarDefineRule = ({
(newQuery: Query) => {
const { query } = field.value as FieldValueQueryBar;
if (!deepEqual(query, newQuery)) {
field.setValue({ ...(field.value as FieldValueQueryBar), query: newQuery });
field.setValue({ ...(field.value as FieldValueQueryBar), query: newQuery, edited: true });
}
},
[field]
Expand All @@ -171,7 +171,11 @@ export const QueryBarDefineRule = ({
(newQuery: Query) => {
const { query } = field.value as FieldValueQueryBar;
if (!deepEqual(query, newQuery)) {
field.setValue({ ...(field.value as FieldValueQueryBar), query: newQuery });
field.setValue({
...(field.value as FieldValueQueryBar),
query: newQuery,
edited: true,
});
}
},
[field]
Expand All @@ -187,6 +191,7 @@ export const QueryBarDefineRule = ({
filters: newSavedQuery.attributes.filters,
query: newSavedQuery.attributes.query,
saved_id: newSavedQuery.id,
edited: true,
});
}
}
Expand Down
18 changes: 16 additions & 2 deletions ...k/plugins/security_solution/public/detections/components/rules/step_define_rule/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ const stepDefineDefaultValue: DefineStepRule = {
query: { query: '', language: 'kuery' },
filters: [],
saved_id: undefined,
edited: false,
},
threatQueryBar: {
query: { query: '*:*', language: 'kuery' },
Expand Down Expand Up @@ -131,7 +132,7 @@ const StepDefineRuleComponent: FC<StepDefineRuleProps> = ({
options: { stripEmptyFields: false },
schema,
});
const { getFields, getFormData, reset, submit } = form;
const { getFields, getFormData, reset, submit, setFieldValue } = form;
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need setFieldValue here, you can instead use [field].setValue as in handleResetIndices below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I think this is a dead variable at this point and I don't use it below anymore so this will be removed.

const [
{
index: formIndex,
Expand Down Expand Up @@ -165,7 +166,20 @@ const StepDefineRuleComponent: FC<StepDefineRuleProps> = ({
// reset form when rule type changes
useEffect(() => {
reset({ resetValues: false });
}, [reset, ruleType]);
if (getFields().queryBar != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use queryBar from line 140? Don't think you have to re-pull it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

EDIT: Nvm, I see reset was called beforehand.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would isolate this logic to a separate useEffect hook. Shared dependencies can cause undesired effects, e.g. isUpdateView changing would now cause reset() to fire.

const queryBar = getFields().queryBar.value as DefineStepRule['queryBar'];
if (queryBar.edited !== true && !isUpdateView) {
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI this edited field is mostly redundant with the built in isPristine property on each field, except that isPristine becomes false when the value changes below, while edited does not.

Perhaps we could use field.isPristine || field.value.query.query === '*:*' to accomplish the same logic, and do away with the edited property?

Copy link
Contributor

Choose a reason for hiding this comment

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

In talking with @FrankHassanabad I wanted to add this to our team discussion. It's becoming increasingly more difficult to make any changes without either 1) breaking another rule type's logic or 2) adding extra validations throughout. It's feeling like a bit of wack-a-mole. I almost want to just silo off each rule type. One example I ran into with preview was that each rule held the state of other rule types (if let's say I visited Threshold and set some state and then switched to custom query) and had to add logic to ensure that it was picking up and ignoring the right parts.

Copy link
Contributor

@marshallmain marshallmain Nov 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@yctercero Would the schemas here help to silo off the rule types in the frontend code? I created them with a similar goal in mind for the backend of making it clear which fields are valid for each rule type, as well as defining which fields are shared across all rule types. They're usable in the frontend as well but I'm not sure if they fully implement the siloing you have in mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

@marshallmain yea, I think they would. It might add more touchpoints in the front, but that may be worth the security of not having to worry that an edit to one rule will break another.

Copy link
Contributor Author

@FrankHassanabad FrankHassanabad Nov 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@ryland, The isPristine field is something I want to use, but there are more odd side effects of how we use the forms where we do a reset({ resetValues: false }); which will reset the isPristine back to true. Even in that block of code if I were to do something like:

  // reset form when rule type changes
  useEffect(() => {
    reset({ resetValues: false });
    if (getFields().queryBar != null) {
      const queryBar = getFields().queryBar.value as DefineStepRule['queryBar'];
      if (
        isThreatMatchRule(ruleType) &&
        (getFields().queryBar.isPristine || queryBar.query.query === '')
      ) {
        setFieldValue('queryBar', {
          ...stepDefineDefaultValue.queryBar,
          query: { ...stepDefineDefaultValue.queryBar.query, query: '*:*' },
        });
      } else if (getFields().queryBar.isPristine || queryBar.query.query === '*:*') {
        setFieldValue('queryBar', stepDefineDefaultValue.queryBar);
      } else {
        setFieldValue('queryBar', queryBar);
      }
    }
  }, [reset, ruleType, setFieldValue, getFields, isUpdateView]);

It still won't work all the way because isPristine will reset back to true and stay true based on two things which is:

  • The reset will turn it back to true
  • Even if this block of code in the else tries to force pristine back to false:
else {
  setFieldValue('queryBar', queryBar);
}

The forms code if it sees the queryBar value is the same as before it will not trigger pristine back to false. So pristine becomes not useful here.

However, to remove the edit flag I added at a trade off of the forms having funny quirks if a user decides to use *:* in a different rule if they're really flipping around between rules a lot before going to the next step would be this:

  // reset form when rule type changes
  useEffect(() => {
    reset({ resetValues: false });
    const queryBar = getFields().queryBar.value as DefineStepRule['queryBar'];
    if (getFields().queryBar != null) {
      if (isThreatMatchRule(ruleType) && queryBar.query.query === '') {
        setFieldValue('queryBar', {
          ...stepDefineDefaultValue.queryBar,
          query: { ...stepDefineDefaultValue.queryBar.query, query: '*:*' },
        });
      } else if (queryBar.query.query === '*:*') {
        setFieldValue('queryBar', stepDefineDefaultValue.queryBar);
      }
    }
  }, [reset, ruleType, setFieldValue, getFields, isUpdateView]);

That would just hardcode the two conditions of either detecting an empty string or detecting a *:* but if a user decides to use *:* and then flip between rules such as threshold and custom query they are going to get the surprise that the *:* turns back into empty string. But the validation would turn on to warn them they have an empty string they need to deal with.

If that is deemed the "the lesser of evils" with what we already have in the forms code I can commit the code above.

Copy link
Contributor Author

@FrankHassanabad FrankHassanabad Nov 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Actually after messing around with it @rylnd I was able to accomplish everything the edited flag could using a combination of the isPristine along with reset and then the setValue that is looking pretty good. I think this is what I am going to go with:

  // reset form when rule type changes
  useEffect(() => {
    reset({ resetValues: false });
    const { queryBar } = getFields();
    if (queryBar != null && !isUpdateView) {
      if (isThreatMatchRule(ruleType) && queryBar.isPristine) {
        queryBar.reset({
          defaultValue: {
            ...stepDefineDefaultValue.queryBar,
            query: { ...stepDefineDefaultValue.queryBar.query, query: '*:*' },
          },
        });
      } else if (queryBar.isPristine) {
        queryBar.reset({
          defaultValue: {
            ...stepDefineDefaultValue.queryBar,
            query: { ...stepDefineDefaultValue.queryBar.query, query: '' },
          },
        });
      } else {
        // This ensures the isPristine is still set to false after a user has modified it
        queryBar.setValue((prev: DefineStepRule['queryBar']) => ({ ...prev }));
      }
    }
  }, [reset, ruleType, getFields, isUpdateView]);

if (isThreatMatchRule(ruleType)) {
setFieldValue('queryBar', {
Copy link
Contributor

Choose a reason for hiding this comment

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

field.setValue() has a format similar to setState where it accepts a function that receives the previous value; that might be more appropriate than merging in the original defaults, here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, see comments above, I can use this in combination with reset and I think we are going to get what we want out of this feature in a more straight forward way.

...stepDefineDefaultValue.queryBar,
query: { ...stepDefineDefaultValue.queryBar.query, query: '*:*' },
});
} else {
setFieldValue('queryBar', stepDefineDefaultValue.queryBar);
}
}
}
}, [reset, ruleType, setFieldValue, getFields, isUpdateView]);

useEffect(() => {
setIndexModified(!isEqual(index, indicesConfig));
Expand Down
5 changes: 2 additions & 3 deletions .../plugins/security_solution/public/detections/components/rules/step_define_rule/schema.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,12 +77,11 @@ export const schema: FormSchema<DefineStepRule> = {
...args: Parameters<ValidationFunc>
): ReturnType<ValidationFunc<{}, ERROR_CODE>> | undefined => {
const [{ value, path, formData }] = args;
const { query, filters } = value as FieldValueQueryBar;
const { query, filters } = value as DefineStepRule['queryBar'];
const needsValidation = !isMlRule(formData.ruleType);
if (!needsValidation) {
return;
}

return isEmpty(query.query as string) && isEmpty(filters)
? {
code: 'ERR_FIELD_MISSING',
Expand All @@ -97,7 +96,7 @@ export const schema: FormSchema<DefineStepRule> = {
...args: Parameters<ValidationFunc>
): ReturnType<ValidationFunc<{}, ERROR_CODE>> | undefined => {
const [{ value, path, formData }] = args;
const { query } = value as FieldValueQueryBar;
const { query } = value as DefineStepRule['queryBar'];
const needsValidation = !isMlRule(formData.ruleType);
if (!needsValidation) {
return;
Expand Down
4 changes: 2 additions & 2 deletions ...ns/security_solution/public/detections/pages/detection_engine/rules/all/__mocks__/mock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,9 @@
import { esFilters } from '../../../../../../../../../../src/plugins/data/public';
import { Rule, RuleError } from '../../../../../containers/detection_engine/rules';
import { AboutStepRule, ActionsStepRule, DefineStepRule, ScheduleStepRule } from '../../types';
import { FieldValueQueryBar } from '../../../../../components/rules/query_bar';
import { fillEmptySeverityMappings } from '../../helpers';

export const mockQueryBar: FieldValueQueryBar = {
export const mockQueryBar: DefineStepRule['queryBar'] = {
query: {
query: 'test query',
language: 'kuery',
Expand Down Expand Up @@ -38,6 +37,7 @@ export const mockQueryBar: FieldValueQueryBar = {
},
],
saved_id: 'test123',
edited: false,
};

export const mockRule = (id: string): Rule => ({
Expand Down
3 changes: 3 additions & 0 deletions ...plugins/security_solution/public/detections/pages/detection_engine/rules/helpers.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ describe('rule helpers', () => {
query: 'user.name: root or user.name: admin',
language: 'kuery',
},
edited: false,
filters: [
{
$state: {
Expand Down Expand Up @@ -220,6 +221,7 @@ describe('rule helpers', () => {
query: '',
language: 'kuery',
},
edited: false,
filters: [],
saved_id: "Garrett's IP",
},
Expand Down Expand Up @@ -262,6 +264,7 @@ describe('rule helpers', () => {
query: '',
language: 'kuery',
},
edited: false,
filters: [],
saved_id: undefined,
},
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/helpers.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ export const getDefineStepsData = (rule: Rule): DefineStepRule => ({
query: { query: rule.query ?? '', language: rule.language ?? '' },
filters: (rule.filters ?? []) as Filter[],
saved_id: rule.saved_id,
edited: false,
},
timeline: {
id: rule.timeline_id ?? null,
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ export interface DefineStepRule {
anomalyThreshold: number;
index: string[];
machineLearningJobId: string;
queryBar: FieldValueQueryBar;
queryBar: FieldValueQueryBar & { edited: boolean };
ruleType: Type;
timeline: FieldValueTimeline;
threshold: FieldValueThreshold;
Expand Down