[Security Solution][Detections] Rule forms cleanup (#76138) (#76827)

* Remove unused isNew field

Digging through the git history, it looks like this was replaced with
the isUpdateView prop at some point. There's a small chance that we're
indirectly leveraging the effect of this value being changed, but I
think we're safe.

* WIP: Making rule form type safe

We have lots of anys and unknowns in here, this is my attempt to fix
that.

I started by defining better types for the state/refs in our parent
component; everything else mostly flowed out of that:

* Step components now type their form hook for their step's data
* Removes lots of unneeded `as` casts
* Renames uses of `accordionId` with `step` and `activeStep`, since they
  are also values of our `RuleStep` enum
* Step components now export their default values
  * The data flow is simpler when the parent passes these values in,
    rather than trying to merge props with some internal defaults.
  * The internal defaulting is still there, but I think it'll be
    unnecessary once I audit the `edit` forms.

I've only done this work for the Define step for now, the rest are next.

* Make defaultValues a required prop of the define step

Now that the create step is passing in the default values, we no longer
need to merge with internal state.

The one exception is the default indexes; since we need that data for
our "reset to default indexes" behavior, we'll keep that functionality
within our DefineStep component.

* Refactor rule creation forms to not require default values

We don't gain much by forcing the parent to pass in default values. The
slightly cleaner types are not worth the burden to the parent; instead,
we add a type guard to be used in our parent to ensure our values are
present before working with them. Previously, we were circumventing this
logic with an `as` cast.

* Remove unnecessary "deep" comparison

These are arrays of strings, so a shallow comparison should suffice
here. Also reorders conditions to short-circuit on simple booleans
first.

* Make StepRuleDescription generic on its schema

* Fixes bug introduced by form lib updates

There's currently a bug on master where returning to a previous form
step does not populate its previous values.

After some investigation it appears that this is due to form values
being reset on submission (form.reset()). Previously, we kept a separate
copy of data in each step's state, and had a useEffect that would
repopulate the form's values if they ever became out of sync. Once that
was removed I believe this bug was introduced.

For now the fix is effectively reimplementing the above behavior, albeit
a little more elegantly with `reset()`. If we can restructure the form
logic to only require the form data at the end (big if), then we can
remove the need to "repopulate" these values to the form.

For now, this does remove the local copy of data in the step component
as I believe that is no longer needed. Data is stored in the parent,
copied/modified to the form, and pushed back up when one clicks on to
the next step.

* Rename typed hook to obviate eslint exception

The linter was complaining because it didn't think that `typedUseKibana`
was a hook. But it is, and we should name it as such.

* WIP: Fixing type errors in the other form steps

Things still aren't quite working, state gets lost when moving through
steps but I believe this is addressed in an outstanding PR so I'm not
sweating it right now.

* Removes as much state in Step components as possible
  * We shouldn't need this as the form holds all the state as well. If
    we need to "watch" for a change, we can subscribe to the form's
    observable to replace FormDataProvider and local state (TODO)
* Removes setting of default values in form components
  * I believe that this is redundant with defaultValues provided to
    useForm, but I need to verify.

* More form cleanup

* Removes redundant uses of field's defaultValue
  * Most are redundant with the form's defaultValue; added calls to
    field.setValue in cases where the default is generated internally
* Removes calls to reset() after submitting
  * These were needed due to a bug in the form lib; once #75796 is
    merged these will no longer be necessary.

* Fix some leftover type errors

* Remove duplicated useEffect hook

This exists identically earlier in the component; I'm guessing it was
the result of a bad merge conflict.

* Fix Rule edit form

* Makes data structures more similar to rule creation form
* Adds type guards for asserting which step is "active"
* Simplifies logic around the active tabe/step/form

* Fixes About Step jest tests

* Removes use of wait() in favor of act()
* Fixes mock call assertion now that we're no longer setting our data to
  null (which was a now-unnecessary form lib workaround).

* Fix bug with going to a previous step after editing actions

We never send our actions data back down to the actions form, so it was
lost if you went to a previous step.

Since the actions UI still had any connectors you created, you merely
had to reselect the throttle and the connector, but this prevents you
from having to do that.

* Add assertions to our rule creation test

Asserts that our rule form repopulates with the provided values when
going back to a previous step. This is to cover a regression that was
not caught by CI (but which has now been fixed).

* Simplify Rule Creation logic

* Validation and data collection are performed in the parent, not the
  step component
* Step component provides a form ref and notifies the parent when it's
  being submitted; the rest is the parent's responsibility
* Renames some internal helper functions to be more declarative:
  submitStep, editStep, etc.

* Don't persist empty form data when leaving a form step

If the active step form is invalid we will receive no data, so we must
not persist that lest the form blows up on absent values when we later
navigate back.

* Skip About Step tests for now

These exercise functionality that was moved into the parent, so they
need a new home.

* Remove unnecessary calls to setValue

* Instead of setting our kibana url after the form is created, we add it
  to the form's default state
* We do not need to set the throttle field value, the field component
  already does this

* Style: logic cleanup

* Prevent users from navigating away from an invalid step on rule edit

We can go against the form lib conventions and persist this invalid data
ourselves on transition, but for now this brings the create/edit forms
into alignment so that adding the aforementioned behavior should be
nearly identical on both.

* Display callout if attempting to navigate away from an invalid tab

We already do this if you try to _submit_ the form, but not when you
click on another tab.

* Persist our form submit() rather than the entire form

Making the entire stateful form a useEffect dependency was likely
causing unnecessary render cycles.

This may also have been part of why both the hooks and the data are refs
instead of normal state; to prevent these rerenders.

* Replace FormDataProvider with useFormData hook

We have to do a type cast here because the hook's data is not typed, but
other than that this is a solid win and cleans things up immensely; the
side effects that result from these values changing are much more
apparent (IMO).

* Move fetch of fields data _after_ form initialization

This ensures that our first fetch of fields will use the index patterns
on the form, not necessarily the default ones.

* Replace FormDataProvider on About step

* This fixes a bug where changing the default severity no longer updated
  the default risk score. It looks like this was broken when the
  severity/riskScore overrides were added, and the values of these
  fields changed from primitives to objects.

* Replace local state with useFormData

By watching the value directly from the form we no longer have a need
for local state, as we were just using it to determine whether the
throttle had changed from the default.

* Types cleanup

Remove some unneeded casts, add some needed ones.

* Rewrite About Step tests

Rather than asserting that the form is invalid through the UI, we
instead retrieve data out of the form hook and assert on that instead.

* Add memoization back to StepRuleDescription

I'm not sure that it's necessary, but best to leave it until we have
time to audit/remove multiple of these.

* Do not fetch ML Jobs if StepRuleDescription is not rendering ML Data

We were incorrectly invoking the useSecurityJobs hook any time the
StepRuleDescription component was rendered, regardless of whether we
actually needed that data.

This moves the useSecurityJobs hook into the component that uses it,
MlJobDescription. If we end up having multiple of these on the page we
can evaluate caching/sharing this data somehow, but for now this
prevents:

* 3x3=9 unnecessary ML calls on the Rule Create page.
* 1x3=3 unnecessary ML calls on Rule Details
* 1x3=3 unnecessary ML Calls on the Rule Edit page.

* Fix bug where revisiting the About step could modify the user's Risk Score

With the severity/risk score link back in place, there was a bug where a
user who had previously set a manual risk score would have it rewritten
on edit (or edit during rule creation).

This was due to a poorly-written useEffect that basically said "if there
is a severity, set a risk score." This has now been amended to say "if
the user changes the severity, set a risk score."

* Clean up About Step tests

* We don't need act(), it's not doing anything.
* We don't need to click Continue since we're just talking to the form
  hook

* Fix local form data when form isn't mounted

If the form isn't on the page (e.g. if we're read-only), then
useFormData will return no values. In these cases, we can simply fall
back to the initialState values, as they'll either be: the default
values on a new form, or: the current values on an active create/edit
form.

Updates the manual type of useFormData to reflect this "maybe" fact.

* Allow user to navigate between invalid tabs on Edit Rule

* Form hooks now _always_ return the form's data, regardless of validity
* Edit Rule now:
  * persists invalid data
  * submits both the active form and the destination form on navigation.
    This is necessary to refresh validations on the destination form,
    since the form lib assumes a newly-mounted form is valid
* simplifies "invalid tab" logic to be derived from our persisted data

* Fix logical error

If the rule is immutable, they can only edit actions.

* Remove unneeded eslint exception

Fixed by upstream #76471

* Make 21 the default risk score for a new rule

Since the default severity is 'low,' these two defaults now coincide.

* Remove duplicated type in favor of common one

Author: rylnd

x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_custom.spec.ts

@@ -58,6 +58,8 @@ import {
   createAndActivateRule,
   fillAboutRuleAndContinue,
   fillDefineCustomRuleWithImportedQueryAndContinue,
+  expectDefineFormToRepopulateAndContinue,
+  expectAboutFormToRepopulateAndContinue,
 } from '../tasks/create_new_rule';
 import { esArchiverLoad, esArchiverUnload } from '../tasks/es_archiver';
 import { loginAndWaitForPageWithoutDateRange } from '../tasks/login';
@@ -82,6 +84,8 @@ describe('Detection rules, custom', () => {
     goToCreateNewRule();
     fillDefineCustomRuleWithImportedQueryAndContinue(newRule);
     fillAboutRuleAndContinue(newRule);
+    expectDefineFormToRepopulateAndContinue(newRule);
+    expectAboutFormToRepopulateAndContinue(newRule);
     createAndActivateRule();
 
     cy.get(CUSTOM_RULES_BTN).invoke('text').should('eql', 'Custom rules (1)');

x-pack/plugins/security_solution/cypress/screens/create_new_rule.ts

@@ -6,6 +6,8 @@
 
 export const ABOUT_CONTINUE_BTN = '[data-test-subj="about-continue"]';
 
+export const ABOUT_EDIT_BUTTON = '[data-test-subj="edit-about-rule"]';
+
 export const ADD_FALSE_POSITIVE_BTN =
   '[data-test-subj="detectionEngineStepAboutRuleFalsePositives"] .euiButtonEmpty__text';
 
@@ -26,6 +28,8 @@ export const CUSTOM_QUERY_INPUT = '[data-test-subj="queryInput"]';
 
 export const DEFINE_CONTINUE_BUTTON = '[data-test-subj="define-continue"]';
 
+export const DEFINE_EDIT_BUTTON = '[data-test-subj="edit-define-rule"]';
+
 export const IMPORT_QUERY_FROM_SAVED_TIMELINE_LINK =
   '[data-test-subj="importQueryFromSavedTimeline"]';
 

x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts

@@ -48,6 +48,8 @@ import {
   THRESHOLD_FIELD_SELECTION,
   THRESHOLD_INPUT_AREA,
   THRESHOLD_TYPE,
+  DEFINE_EDIT_BUTTON,
+  ABOUT_EDIT_BUTTON,
 } from '../screens/create_new_rule';
 import { TIMELINE } from '../screens/timeline';
 
@@ -175,6 +177,20 @@ export const fillDefineCustomRuleWithImportedQueryAndContinue = (
   cy.get(CUSTOM_QUERY_INPUT).should('not.exist');
 };
 
+export const expectDefineFormToRepopulateAndContinue = (rule: CustomRule) => {
+  cy.get(DEFINE_EDIT_BUTTON).click();
+  cy.get(CUSTOM_QUERY_INPUT).invoke('text').should('eq', rule.customQuery);
+  cy.get(DEFINE_CONTINUE_BUTTON).should('exist').click({ force: true });
+  cy.get(DEFINE_CONTINUE_BUTTON).should('not.exist');
+};
+
+export const expectAboutFormToRepopulateAndContinue = (rule: CustomRule) => {
+  cy.get(ABOUT_EDIT_BUTTON).click();
+  cy.get(RULE_NAME_INPUT).invoke('val').should('eq', rule.name);
+  cy.get(ABOUT_CONTINUE_BTN).should('exist').click({ force: true });
+  cy.get(ABOUT_CONTINUE_BTN).should('not.exist');
+};
+
 export const fillDefineThresholdRuleAndContinue = (rule: ThresholdRule) => {
   const thresholdField = 0;
   const threshold = 1;

x-pack/plugins/security_solution/public/common/lib/kibana/kibana_react.ts

@@ -19,12 +19,11 @@ export interface WithKibanaProps {
   kibana: KibanaContext;
 }
 
-// eslint-disable-next-line react-hooks/rules-of-hooks
-const typedUseKibana = () => useKibana<StartServices>();
+const useTypedKibana = () => useKibana<StartServices>();
 
 export {
   KibanaContextProvider,
-  typedUseKibana as useKibana,
+  useTypedKibana as useKibana,
   useUiSetting,
   useUiSetting$,
   withKibana,

x-pack/plugins/security_solution/public/detections/components/rules/description_step/index.test.tsx

@@ -7,7 +7,7 @@ import React from 'react';
 import { shallow, mount } from 'enzyme';
 
 import {
-  StepRuleDescriptionComponent,
+  StepRuleDescription,
   addFilterStateIfNotThere,
   buildListItems,
   getDescriptionItem,
@@ -52,24 +52,24 @@ describe('description_step', () => {
     mockAboutStep = mockAboutStepRule();
   });
 
-  describe('StepRuleDescriptionComponent', () => {
+  describe('StepRuleDescription', () => {
     test('renders tow columns when "columns" is "multi"', () => {
       const wrapper = shallow(
-        <StepRuleDescriptionComponent columns="multi" data={mockAboutStep} schema={schema} />
+        <StepRuleDescription columns="multi" data={mockAboutStep} schema={schema} />
       );
       expect(wrapper.find('[data-test-subj="listItemColumnStepRuleDescription"]')).toHaveLength(2);
     });
 
     test('renders single column when "columns" is "single"', () => {
       const wrapper = shallow(
-        <StepRuleDescriptionComponent columns="single" data={mockAboutStep} schema={schema} />
+        <StepRuleDescription columns="single" data={mockAboutStep} schema={schema} />
       );
       expect(wrapper.find('[data-test-subj="listItemColumnStepRuleDescription"]')).toHaveLength(1);
     });
 
     test('renders one column with title and description split when "columns" is "singleSplit', () => {
       const wrapper = shallow(
-        <StepRuleDescriptionComponent columns="singleSplit" data={mockAboutStep} schema={schema} />
+        <StepRuleDescription columns="singleSplit" data={mockAboutStep} schema={schema} />
       );
       expect(wrapper.find('[data-test-subj="listItemColumnStepRuleDescription"]')).toHaveLength(1);
       expect(
@@ -299,7 +299,6 @@ describe('description_step', () => {
     describe('queryBar', () => {
       test('returns array of ListItems when queryBar exist', () => {
         const mockQueryBar = {
-          isNew: false,
           queryBar: {
             query: {
               query: 'user.name: root or user.name: admin',
@@ -369,7 +368,6 @@ describe('description_step', () => {
     describe('threshold', () => {
       test('returns threshold description when threshold exist and field is empty', () => {
         const mockThreshold = {
-          isNew: false,
           threshold: {
             field: [''],
             value: 100,
@@ -391,7 +389,6 @@ describe('description_step', () => {
 
       test('returns threshold description when threshold exist and field is set', () => {
         const mockThreshold = {
-          isNew: false,
           threshold: {
             field: ['user.name'],
             value: 100,

x-pack/plugins/security_solution/public/detections/components/rules/description_step/index.tsx

@@ -37,7 +37,6 @@ import {
   buildRuleTypeDescription,
   buildThresholdDescription,
 } from './helpers';
-import { useSecurityJobs } from '../../../../common/components/ml_popover/hooks/use_security_jobs';
 import { buildMlJobDescription } from './ml_job_description';
 import { buildActionsDescription } from './actions_description';
 import { buildThrottleDescription } from './throttle_description';
@@ -52,22 +51,21 @@ const DescriptionListContainer = styled(EuiDescriptionList)`
   }
 `;
 
-interface StepRuleDescriptionProps {
+interface StepRuleDescriptionProps<T> {
   columns?: 'multi' | 'single' | 'singleSplit';
   data: unknown;
   indexPatterns?: IIndexPattern;
-  schema: FormSchema;
+  schema: FormSchema<T>;
 }
 
-export const StepRuleDescriptionComponent: React.FC<StepRuleDescriptionProps> = ({
+export const StepRuleDescriptionComponent = <T,>({
   data,
   columns = 'multi',
   indexPatterns,
   schema,
-}) => {
+}: StepRuleDescriptionProps<T>) => {
   const kibana = useKibana();
   const [filterManager] = useState<FilterManager>(new FilterManager(kibana.services.uiSettings));
-  const { jobs } = useSecurityJobs(false);
 
   const keys = Object.keys(schema);
   const listItems = keys.reduce((acc: ListItems[], key: string) => {
@@ -76,8 +74,7 @@ export const StepRuleDescriptionComponent: React.FC<StepRuleDescriptionProps> =
         ...acc,
         buildMlJobDescription(
           get(key, data) as string,
-          (get(key, schema) as { label: string }).label,
-          jobs
+          (get(key, schema) as { label: string }).label
         ),
       ];
     }
@@ -125,11 +122,13 @@ export const StepRuleDescriptionComponent: React.FC<StepRuleDescriptionProps> =
   );
 };
 
-export const StepRuleDescription = memo(StepRuleDescriptionComponent);
+export const StepRuleDescription = memo(
+  StepRuleDescriptionComponent
+) as typeof StepRuleDescriptionComponent;
 
-export const buildListItems = (
+export const buildListItems = <T,>(
   data: unknown,
-  schema: FormSchema,
+  schema: FormSchema<T>,
   filterManager: FilterManager,
   indexPatterns?: IIndexPattern
 ): ListItems[] =>

x-pack/plugins/security_solution/public/detections/components/rules/description_step/ml_job_description.test.tsx

@@ -14,7 +14,7 @@ jest.mock('../../../../common/lib/kibana');
 
 describe('MlJobDescription', () => {
   it('renders correctly', () => {
-    const wrapper = shallow(<MlJobDescription job={mockOpenedJob} />);
+    const wrapper = shallow(<MlJobDescription jobId={'myJobId'} />);
 
     expect(wrapper.find('[data-test-subj="machineLearningJobId"]')).toHaveLength(1);
   });

x-pack/plugins/security_solution/public/detections/components/rules/description_step/ml_job_description.tsx

@@ -10,6 +10,7 @@ import { EuiBadge, EuiIcon, EuiLink, EuiToolTip } from '@elastic/eui';
 
 import { MlSummaryJob } from '../../../../../../ml/public';
 import { isJobStarted } from '../../../../../common/machine_learning/helpers';
+import { useSecurityJobs } from '../../../../common/components/ml_popover/hooks/use_security_jobs';
 import { useKibana } from '../../../../common/lib/kibana';
 import { ListItems } from './types';
 import { ML_JOB_STARTED, ML_JOB_STOPPED } from './translations';
@@ -69,35 +70,33 @@ const Wrapper = styled.div`
   overflow: hidden;
 `;
 
-const MlJobDescriptionComponent: React.FC<{ job: MlSummaryJob }> = ({ job }) => {
+const MlJobDescriptionComponent: React.FC<{ jobId: string }> = ({ jobId }) => {
+  const { jobs } = useSecurityJobs(false);
   const jobUrl = useKibana().services.application.getUrlForApp(
-    `ml#/jobs?mlManagement=(jobId:${encodeURI(job.id)})`
+    `ml#/jobs?mlManagement=(jobId:${encodeURI(jobId)})`
   );
+  const job = jobs.find(({ id }) => id === jobId);
 
-  return (
+  const jobIdSpan = <span data-test-subj="machineLearningJobId">{jobId}</span>;
+
+  return job != null ? (
     <Wrapper>
       <div>
-        <JobLink data-test-subj="machineLearningJobId" href={jobUrl} target="_blank">
-          {job.id}
+        <JobLink href={jobUrl} target="_blank">
+          {jobIdSpan}
         </JobLink>
         <AuditIcon message={job.auditMessage} />
       </div>
       <JobStatusBadge job={job} />
     </Wrapper>
+  ) : (
+    jobIdSpan
   );
 };
 
 export const MlJobDescription = React.memo(MlJobDescriptionComponent);
 
-export const buildMlJobDescription = (
-  jobId: string,
-  label: string,
-  jobs: MlSummaryJob[]
-): ListItems => {
-  const job = jobs.find(({ id }) => id === jobId);
-
-  return {
-    title: label,
-    description: job ? <MlJobDescription job={job} /> : jobId,
-  };
-};
+export const buildMlJobDescription = (jobId: string, label: string): ListItems => ({
+  title: label,
+  description: <MlJobDescription jobId={jobId} />,
+});

x-pack/plugins/security_solution/public/detections/components/rules/next_step/index.tsx

@@ -9,7 +9,7 @@ import { EuiHorizontalRule, EuiFlexGroup, EuiFlexItem, EuiButton } from '@elasti
 import * as RuleI18n from '../../../pages/detection_engine/rules/translations';
 
 interface NextStepProps {
-  onClick: () => Promise<void>;
+  onClick: () => void;
   isDisabled: boolean;
   dataTestSubj?: string;
 }

x-pack/plugins/security_solution/public/detections/components/rules/step_about_rule/data.tsx

@@ -8,12 +8,12 @@ import styled from 'styled-components';
 import { EuiHealth } from '@elastic/eui';
 import euiLightVars from '@elastic/eui/dist/eui_theme_light.json';
 import React from 'react';
-import * as I18n from './translations';
 
-export type SeverityValue = 'low' | 'medium' | 'high' | 'critical';
+import { Severity } from '../../../../../common/detection_engine/schemas/common/schemas';
+import * as I18n from './translations';
 
 export interface SeverityOptionItem {
-  value: SeverityValue;
+  value: Severity;
   inputDisplay: React.ReactElement;
 }
 
@@ -44,7 +44,7 @@ export const severityOptions: SeverityOptionItem[] = [
   },
 ];
 
-export const defaultRiskScoreBySeverity: Record<SeverityValue, number> = {
+export const defaultRiskScoreBySeverity: Record<Severity, number> = {
   low: 21,
   medium: 47,
   high: 73,