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

[FrankHassanabad]

Summary

Allows for Indicator matches to have a default of *:* for the query field when it is selected.

Before, indicator query is blank when first selecting the rule:

After, indicator query is by default *:* unless the user has previously edited the query field:

Adds a stable reference for threat matching to determine when the query field has been modified or not. This is keep the current behavior and the rules operate like this:

• If you select an indicator match rule and nothing has been previously edited it will select *:* for the query
• If you have modified your custom query and select indicator match rule, then *:* will be replaced with that custom query and *:* will not be used.
• If you select EQL rule and then back to this rule type the *:* will be re-inserted and edit: true will flip back to false, due to the magic that is keys within React and how the EQL rule type relies on that.

Checklist

Delete any items that are not applicable to this PR.

• Unit or functional tests were updated or added to match the most common scenarios
• Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
• Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
• This was checked for cross-browser compatibility

[FrankHassanabad]

@elasticmachine merge upstream

[yctercero]

Just started reviewing :)

[yctercero]

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

[yctercero]

EDIT: Nvm, I see reset was called beforehand.

[yctercero]

This might be more a UX question, but do we want KQL queries carried over into the EQL input?

I had selected Threshold Rule, added a query, then selected EQL Rule.

[yctercero]

It seems related to my last comment. But found this little weirdness.

[yctercero]

Just kidding! 🃏 After testing on master, it appears that the two issues I mentioned above are existing issues that may require some team/UX discussion. I'll go ahead and open an issue for this for the team to look at.

LGTM! Pulled down and tried running through various corner cases. New code works great!

[rylnd]

Added some comments revolving around usage of the hook form lib; I think we can do away with the new edited property but it's possible I'm misunderstanding.

[rylnd]

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.

[rylnd]

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

[FrankHassanabad]

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

[rylnd]

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?

[yctercero]

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.

[marshallmain]

@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.

• Those schemas are part of my refactoring PR

[yctercero]

@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.

[FrankHassanabad]

@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.

[FrankHassanabad]

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]);

[rylnd]

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.

[FrankHassanabad]

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.

[rylnd]

Looks great, Frank! Thanks for the followup!

[rylnd]

I was suspicious of this clause being hit if the user manually enters *:* as a query, but it looks to be good since this is a comparison of object literals (which will only be equal in the use of reset() above) and not just values 👍

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: ed94de2

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
securitySolution	7.8MB	7.8MB	+698.0B

History

• 💛 Build #87458 was flaky abdaa53
• 💚 Build #86764 succeeded 7bbc2a1
• 💚 Build #86499 succeeded 6ac90aa
• 💚 Build #86296 succeeded eb32912
• 💚 Build #85645 succeeded 9f98bd6

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

[yctercero]

Re-LGTM :) Tested out again and it looks great! Awesome how you were able to minimize the amount of code changes.