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 Solution] Add UI incentivizers to upgrade prebuilt rules #211862

Merged
merged 21 commits into from
Mar 5, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -145,5 +145,6 @@ export interface ThreeWayDiff<TValue> {
* Given the three versions of a value, calculates a three-way diff for it.
*/
export type ThreeWayDiffAlgorithm<TValue> = (
versions: ThreeVersionsOf<TValue>
versions: ThreeVersionsOf<TValue>,
isRuleCustomized: boolean
) => ThreeWayDiff<TValue>;
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { SortOrder, type RuleObjectId, type RuleSignatureId, type RuleTagArray }
import type { PartialRuleDiff } from '../model';
import type { RuleResponse, RuleVersion } from '../../model/rule_schema';
import { FindRulesSortField } from '../../rule_management';
import { PrebuiltRulesFilter } from '../common/prebuilt_rules_filter';
import { ReviewPrebuiltRuleUpgradeFilter } from '../common/review_prebuilt_rules_upgrade_filter';

export type ReviewRuleUpgradeSort = z.infer<typeof ReviewRuleUpgradeSort>;
export const ReviewRuleUpgradeSort = z.object({
Expand All @@ -27,7 +27,7 @@ export const ReviewRuleUpgradeSort = z.object({
export type ReviewRuleUpgradeRequestBody = z.infer<typeof ReviewRuleUpgradeRequestBody>;
export const ReviewRuleUpgradeRequestBody = z
.object({
filter: PrebuiltRulesFilter.optional(),
filter: ReviewPrebuiltRuleUpgradeFilter.optional(),
sort: ReviewRuleUpgradeSort.optional(),

page: z.coerce.number().int().min(1).optional().default(1),
Expand Down Expand Up @@ -89,4 +89,5 @@ export interface RuleUpgradeInfoForReview {
target_rule: RuleResponse;
diff: PartialRuleDiff;
revision: number;
has_base_version: boolean;
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/

import { ecsFieldMap } from '@kbn/alerts-as-data-utils';
import type { RequiredField, RequiredFieldInput } from '../../api/detection_engine';
import type { RequiredField, RequiredFieldInput, RuleResponse } from '../../api/detection_engine';

/*
Computes the boolean "ecs" property value for each required field based on the ECS field map.
Expand All @@ -23,3 +23,6 @@ export const addEcsToRequiredFields = (requiredFields?: RequiredFieldInput[]): R
ecs: isEcsField,
};
});

export const isRuleCustomized = (rule: RuleResponse) =>
rule.rule_source.type === 'external' && rule.rule_source.is_customized === true;
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
EuiCallOut,
EuiFlexGroup,
EuiFlexItem,
EuiLink,
EuiResizableContainer,
EuiSpacer,
EuiTab,
Expand Down Expand Up @@ -75,6 +76,7 @@ import { usePrebuiltRulesCustomizationStatus } from '../../../rule_management/lo
import { PrebuiltRulesCustomizationDisabledReason } from '../../../../../common/detection_engine/prebuilt_rules/prebuilt_rule_customization_status';
import { ALERT_SUPPRESSION_FIELDS_FIELD_NAME } from '../../../rule_creation/components/alert_suppression_edit';
import { usePrebuiltRuleCustomizationUpsellingMessage } from '../../../rule_management/logic/prebuilt_rules/use_prebuilt_rule_customization_upselling_message';
import { useRuleUpdateCallout } from '../../../rule_management/hooks/use_rule_update_callout';

const EditRulePageComponent: FC<{ rule: RuleResponse }> = ({ rule }) => {
const { addSuccess } = useAppToasts();
Expand Down Expand Up @@ -509,6 +511,16 @@ const EditRulePageComponent: FC<{ rule: RuleResponse }> = ({ rule }) => {
[navigateToApp, ruleId]
);

const upgradeCallout = useRuleUpdateCallout({
Copy link
Contributor

Choose a reason for hiding this comment

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

We mostly stick to upgrade word in the codebase while using update in texts exposed to users. I recommend to stick to upgrade in names you used for consistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

what is a benefit of using hook here, not a component?

rule,
message: ruleI18n.HAS_RULE_UPDATE_EDITING_CALLOUT_MESSAGE,
actionButton: (
<EuiLink onClick={goToDetailsRule} data-test-subj="ruleEditingUpdateRuleCalloutButton">
{ruleI18n.HAS_RULE_UPDATE_EDITING_CALLOUT_BUTTON}
</EuiLink>
),
});

if (
redirectToDetections(
isSignalIndexExists,
Expand Down Expand Up @@ -550,6 +562,7 @@ const EditRulePageComponent: FC<{ rule: RuleResponse }> = ({ rule }) => {
setIsRulePreviewVisible={setIsRulePreviewVisible}
togglePanel={togglePanel}
/>
{upgradeCallout}
{invalidSteps.length > 0 && (
<EuiCallOut title={i18n.SORRY_ERRORS} color="danger" iconType="warning">
<FormattedMessage
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ import { useManualRuleRunConfirmation } from '../../../rule_gaps/components/manu
import { useLegacyUrlRedirect } from './use_redirect_legacy_url';
import { RuleDetailTabs, useRuleDetailsTabs } from './use_rule_details_tabs';
import { useIsExperimentalFeatureEnabled } from '../../../../common/hooks/use_experimental_features';
import { useRuleUpdateCallout } from '../../../rule_management/hooks/use_rule_update_callout';

const RULE_EXCEPTION_LIST_TYPES = [
ExceptionListTypeEnum.DETECTION,
Expand Down Expand Up @@ -254,7 +255,7 @@ const RuleDetailsPageComponent: React.FC<DetectionEngineComponentProps> = ({

const { pollForSignalIndex } = useSignalHelpers();
const [rule, setRule] = useState<RuleResponse | null>(null);
const isLoading = ruleLoading && rule == null;
const isLoading = useMemo(() => ruleLoading && rule == null, [rule, ruleLoading]);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Calculation is pretty trivial here and resulting in a boolean value. It's not necessary to wrap it into useMemo().


const { starting: isStartingJobs, startMlJobs } = useStartMlJobs();
const startMlJobsIfNeeded = useCallback(async () => {
Expand Down Expand Up @@ -394,6 +395,12 @@ const RuleDetailsPageComponent: React.FC<DetectionEngineComponentProps> = ({
const lastExecutionDate = lastExecution?.date ?? '';
const lastExecutionMessage = lastExecution?.message ?? '';

const upgradeCallout = useRuleUpdateCallout({
rule,
message: ruleI18n.HAS_RULE_UPDATE_DETAILS_CALLOUT_MESSAGE,
onUpgrade: refreshRule,
});

const ruleStatusInfo = useMemo(() => {
return (
<>
Expand Down Expand Up @@ -555,6 +562,7 @@ const RuleDetailsPageComponent: React.FC<DetectionEngineComponentProps> = ({
<>
<NeedAdminForUpdateRulesCallOut />
<MissingPrivilegesCallOut />
{upgradeCallout}
{isBulkDuplicateConfirmationVisible && (
<BulkActionDuplicateExceptionsConfirmation
onCancel={cancelRuleDuplication}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { EuiCallOut, EuiSpacer, EuiLink } from '@elastic/eui';
import React, { useMemo } from 'react';
import type { RuleResponse } from '../../../../../common/api/detection_engine';
import * as i18n from './translations';
import { usePrebuiltRulesUpgrade } from '../../hooks/use_prebuilt_rules_upgrade';

interface RuleUpdateCalloutProps {
rule: RuleResponse;
message: string;
actionButton?: JSX.Element;
onUpgrade?: () => void;
}

const RuleUpdateCalloutComponent = ({
rule,
message,
actionButton,
onUpgrade,
}: RuleUpdateCalloutProps): JSX.Element | null => {
const { upgradeReviewResponse, rulePreviewFlyout, openRulePreview } = usePrebuiltRulesUpgrade({
pagination: {
page: 1, // we only want to fetch one result
perPage: 1,
},
filter: { rule_ids: [rule.id] },
onUpgrade,
});

const isRuleUpgradeable = useMemo(
() => upgradeReviewResponse !== undefined && upgradeReviewResponse.total > 0,
[upgradeReviewResponse]
);

const updateCallToActionButton = useMemo(() => {
if (actionButton) {
return actionButton;
}
return (
<EuiLink
onClick={() => openRulePreview(rule.rule_id)}
data-test-subj="ruleDetailsUpdateRuleCalloutButton"
>
{i18n.HAS_RULE_UPDATE_CALLOUT_BUTTON}
</EuiLink>
);
}, [actionButton, openRulePreview, rule.rule_id]);

if (!isRuleUpgradeable) {
return null;
}

return (
<>
<EuiCallOut title={i18n.HAS_RULE_UPDATE_CALLOUT_TITLE} color="primary" iconType="gear">
Copy link
Contributor

Choose a reason for hiding this comment

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

Mixing RuleUpdateCallout with prebuilt rule upgrade flyout isn't the best approach since it makes it harder to reason about code's behaviour without diving into component's implementation. I recommend to do the following

  • move all logic to useRuleUpdateCallout hook
  • rename useRuleUpdateCallout to useRuleUpgrade or useRuleUpgradeUI
  • make renamed useRuleUpdateCallout returning rule upgrade callout and prebuilt rule upgrade flyout separately
  • leave only UI in RuleUpdateCallout component

<p>{message}</p>
{updateCallToActionButton}
</EuiCallOut>
<EuiSpacer size="l" />
{rulePreviewFlyout}
</>
);
};

export const RuleUpdateCallout = React.memo(RuleUpdateCalloutComponent);
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React from 'react';
import { EuiCallOut } from '@elastic/eui';
import * as i18n from './translations';

export const RuleHasMissingBaseVersionCallout = () => (
<EuiCallOut color="warning" size="s">
<p>{i18n.RULE_BASE_VERSION_IS_MISSING_DESCRIPTION}</p>
</EuiCallOut>
);
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { RuleUpgradeInfoBar } from './rule_upgrade_info_bar';
import { RuleUpgradeCallout } from './rule_upgrade_callout';
import { FieldUpgrade } from './field_upgrade';
import { FieldUpgradeContextProvider } from './field_upgrade_context';
import { RuleHasMissingBaseVersionCallout } from './missing_base_version_callout';

interface RuleUpgradeProps {
ruleUpgradeState: RuleUpgradeState;
Expand Down Expand Up @@ -45,6 +46,12 @@ export const RuleUpgrade = memo(function RuleUpgrade({
targetVersionNumber={ruleUpgradeState.target_rule.version}
/>
<EuiSpacer size="s" />
{!ruleUpgradeState.has_base_version && (
<>
<RuleHasMissingBaseVersionCallout />
<EuiSpacer size="s" />
</>
)}
<RuleUpgradeCallout
numOfSolvableConflicts={numOfSolvableConflicts}
numOfNonSolvableConflicts={numOfNonSolvableConflicts}
Expand Down
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's roll back changes in this files. It looks like only fragment wrappers left here.

Original file line number Diff line number Diff line change
Expand Up @@ -23,66 +23,72 @@ export function RuleUpgradeCallout({
}: RuleUpgradeCalloutProps): JSX.Element {
if (numOfNonSolvableConflicts > 0) {
return (
<EuiCallOut
title={
<>
<strong>{i18n.UPGRADE_STATUS}</strong>
&nbsp;
<ActionRequiredBadge />
&nbsp;
{i18n.RULE_HAS_CONFLICTS(numOfNonSolvableConflicts + numOfSolvableConflicts)}
</>
}
color="danger"
size="s"
>
<span>{i18n.RULE_HAS_HARD_CONFLICTS_DESCRIPTION}</span>
<ul>
<li>{i18n.RULE_HAS_HARD_CONFLICTS_KEEP_YOUR_CHANGES}</li>
<li>{i18n.RULE_HAS_HARD_CONFLICTS_ACCEPT_ELASTIC_UPDATE}</li>
<li>{i18n.RULE_HAS_HARD_CONFLICTS_EDIT_FINAL_VERSION}</li>
</ul>
</EuiCallOut>
<>
<EuiCallOut
title={
<>
<strong>{i18n.UPGRADE_STATUS}</strong>
&nbsp;
<ActionRequiredBadge />
&nbsp;
{i18n.RULE_HAS_CONFLICTS(numOfNonSolvableConflicts + numOfSolvableConflicts)}
</>
}
color="danger"
size="s"
>
<span>{i18n.RULE_HAS_HARD_CONFLICTS_DESCRIPTION}</span>
<ul>
<li>{i18n.RULE_HAS_HARD_CONFLICTS_KEEP_YOUR_CHANGES}</li>
<li>{i18n.RULE_HAS_HARD_CONFLICTS_ACCEPT_ELASTIC_UPDATE}</li>
<li>{i18n.RULE_HAS_HARD_CONFLICTS_EDIT_FINAL_VERSION}</li>
</ul>
</EuiCallOut>
</>
);
}

if (numOfSolvableConflicts > 0) {
return (
<>
<EuiCallOut
title={
<>
<strong>{i18n.UPGRADE_STATUS}</strong>
&nbsp;
<ReviewRequiredBadge />
&nbsp;
{i18n.RULE_HAS_CONFLICTS(numOfSolvableConflicts)}
</>
}
color="warning"
size="s"
>
<span>{i18n.RULE_HAS_SOFT_CONFLICTS_DESCRIPTION}</span>
<ul>
<li>{i18n.RULE_HAS_SOFT_CONFLICTS_ACCEPT_SUGGESTED_UPDATE}</li>
<li>{i18n.RULE_HAS_SOFT_CONFLICTS_EDIT_FINAL_VERSION}</li>
</ul>
</EuiCallOut>
</>
);
}

return (
<>
<EuiCallOut
title={
<>
<strong>{i18n.UPGRADE_STATUS}</strong>
&nbsp;
<ReviewRequiredBadge />
&nbsp;
{i18n.RULE_HAS_CONFLICTS(numOfSolvableConflicts)}
<ReadyForUpgradeBadge />
</>
}
color="warning"
color="success"
size="s"
>
<span>{i18n.RULE_HAS_SOFT_CONFLICTS_DESCRIPTION}</span>
<ul>
<li>{i18n.RULE_HAS_SOFT_CONFLICTS_ACCEPT_SUGGESTED_UPDATE}</li>
<li>{i18n.RULE_HAS_SOFT_CONFLICTS_EDIT_FINAL_VERSION}</li>
</ul>
<p>{i18n.RULE_IS_READY_FOR_UPGRADE_DESCRIPTION}</p>
</EuiCallOut>
);
}

return (
<EuiCallOut
title={
<>
<strong>{i18n.UPGRADE_STATUS}</strong>
&nbsp;
<ReadyForUpgradeBadge />
</>
}
color="success"
size="s"
>
<p>{i18n.RULE_IS_READY_FOR_UPGRADE_DESCRIPTION}</p>
</EuiCallOut>
</>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -158,3 +158,11 @@ export const FIELD_MODIFIED_BADGE_DESCRIPTION = i18n.translate(
'This field value differs from the one provided in the original version of the rule.',
}
);

export const RULE_BASE_VERSION_IS_MISSING_DESCRIPTION = i18n.translate(
'xpack.securitySolution.detectionEngine.upgradeFlyout.baseVersionMissingDescription',
{
defaultMessage:
"The original, unedited version of this Elastic rule couldn't be found. This sometimes happens when a rule hasn't been updated in a while. You can still update this rule, but will only have access to its current version and the incoming Elastic update. Updating Elastic rules more often can help you avoid this in the future. We encourage you to review this update carefully and ensure your changes are not accidentally overwritten.",
}
);
Original file line number Diff line number Diff line change
Expand Up @@ -461,3 +461,17 @@ export const LUCENE_LANGUAGE_LABEL = i18n.translate(
defaultMessage: 'Lucene',
}
);

export const HAS_RULE_UPDATE_CALLOUT_TITLE = i18n.translate(
'xpack.securitySolution.detectionEngine.ruleDetails.updateCalloutTitle',
{
defaultMessage: 'Elastic rule update available',
}
);

export const HAS_RULE_UPDATE_CALLOUT_BUTTON = i18n.translate(
'xpack.securitySolution.detectionEngine.ruleDetailsUpdate.calloutButton',
{
defaultMessage: 'Review update',
}
);
Loading