Skip to content

Commit

Permalink
[8.15] [Security Solution] Fix non-responsive rule details page (#187953
Browse files Browse the repository at this point in the history
) (#188305)

# Backport

This will backport the following commits from `main` to `8.15`:
- [[Security Solution] Fix non-responsive rule details page
(#187953)](#187953)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Maxim
Palenov","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-07-15T11:37:26Z","message":"[Security
Solution] Fix non-responsive rule details page
(#187953)\n\n**Resolves:**
https://github.com/elastic/kibana/issues/177734\r\n\r\n##
Summary\r\n\r\nThis PR fixes a non-responsive rule details page under
non default space.\r\n\r\n## Details\r\n\r\n**[Security Solution] Rule
details page is not responsive and leads to page crash for rule in
non-default spaces
[#177734](#177734 resurfaced
back. Investigation has show that **[Security Solution] Remove usage of
deprecated React rendering utilities
[#181099](#181099 is the
cause.\r\n\r\nThe problem is quite subtle to comprehend it just by
looking at the code. In fact it boils down to an unstable `useAsync()`
hook dependency. Every re-render `useAsync()` resolves a promise causing
an additional re-render to show updated results and the cycle repeats.
Such hook is used in
`x-pack/plugins/security_solution/public/common/components/visualization_actions/actions.tsx`\r\n\r\n```ts\r\n
const panels = useAsync(\r\n () =>\r\n buildContextMenuForActions({\r\n
actions: contextMenuActions.map((action) => ({\r\n action,\r\n context:
{},\r\n trigger: VISUALIZATION_CONTEXT_MENU_TRIGGER,\r\n })),\r\n
}),\r\n [contextMenuActions]\r\n );\r\n```\r\n\r\nwhere
`contextMenuActions` is an unstable dependency. This is the case due to
refactoring to `useSaveToLibrary()` hook by **[Security Solution] Remove
usage of deprecated React rendering utilities
[#181099](#181099 which started
retuning a new object every render. The dependency chain is
`contextMenuActions` -> `useActions()` ->
`useSaveToLibrary()`.\r\n\r\nThe actual fix is to
replace\r\n\r\n```ts\r\nconst { lens, ...startServices } =
useKibana().services;\r\n```\r\n\r\nwith\r\n\r\n```ts\r\nconst
startServices = useKibana().services;\r\n```\r\n\r\nSince
`startServices` is used as a hook dependency it must be stable. A rest
property in object destruction expression is always a new object and
can't be used as a dependency as is. Using stable `useKibana().services`
fixes the
problem.","sha":"8a539a8a4ce483d2e5d3aa484d8ec78887f338b8","branchLabelMapping":{"^v8.16.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","impact:critical","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection Rule
Management","Feature:Rule
Details","v8.15.0","v8.16.0"],"title":"[Security Solution] Fix
non-responsive rule details
page","number":187953,"url":"https://github.com/elastic/kibana/pull/187953","mergeCommit":{"message":"[Security
Solution] Fix non-responsive rule details page
(#187953)\n\n**Resolves:**
https://github.com/elastic/kibana/issues/177734\r\n\r\n##
Summary\r\n\r\nThis PR fixes a non-responsive rule details page under
non default space.\r\n\r\n## Details\r\n\r\n**[Security Solution] Rule
details page is not responsive and leads to page crash for rule in
non-default spaces
[#177734](#177734 resurfaced
back. Investigation has show that **[Security Solution] Remove usage of
deprecated React rendering utilities
[#181099](#181099 is the
cause.\r\n\r\nThe problem is quite subtle to comprehend it just by
looking at the code. In fact it boils down to an unstable `useAsync()`
hook dependency. Every re-render `useAsync()` resolves a promise causing
an additional re-render to show updated results and the cycle repeats.
Such hook is used in
`x-pack/plugins/security_solution/public/common/components/visualization_actions/actions.tsx`\r\n\r\n```ts\r\n
const panels = useAsync(\r\n () =>\r\n buildContextMenuForActions({\r\n
actions: contextMenuActions.map((action) => ({\r\n action,\r\n context:
{},\r\n trigger: VISUALIZATION_CONTEXT_MENU_TRIGGER,\r\n })),\r\n
}),\r\n [contextMenuActions]\r\n );\r\n```\r\n\r\nwhere
`contextMenuActions` is an unstable dependency. This is the case due to
refactoring to `useSaveToLibrary()` hook by **[Security Solution] Remove
usage of deprecated React rendering utilities
[#181099](#181099 which started
retuning a new object every render. The dependency chain is
`contextMenuActions` -> `useActions()` ->
`useSaveToLibrary()`.\r\n\r\nThe actual fix is to
replace\r\n\r\n```ts\r\nconst { lens, ...startServices } =
useKibana().services;\r\n```\r\n\r\nwith\r\n\r\n```ts\r\nconst
startServices = useKibana().services;\r\n```\r\n\r\nSince
`startServices` is used as a hook dependency it must be stable. A rest
property in object destruction expression is always a new object and
can't be used as a dependency as is. Using stable `useKibana().services`
fixes the
problem.","sha":"8a539a8a4ce483d2e5d3aa484d8ec78887f338b8"}},"sourceBranch":"main","suggestedTargetBranches":["8.15"],"targetPullRequestStates":[{"branch":"8.15","label":"v8.15.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/187953","number":187953,"mergeCommit":{"message":"[Security
Solution] Fix non-responsive rule details page
(#187953)\n\n**Resolves:**
https://github.com/elastic/kibana/issues/177734\r\n\r\n##
Summary\r\n\r\nThis PR fixes a non-responsive rule details page under
non default space.\r\n\r\n## Details\r\n\r\n**[Security Solution] Rule
details page is not responsive and leads to page crash for rule in
non-default spaces
[#177734](#177734 resurfaced
back. Investigation has show that **[Security Solution] Remove usage of
deprecated React rendering utilities
[#181099](#181099 is the
cause.\r\n\r\nThe problem is quite subtle to comprehend it just by
looking at the code. In fact it boils down to an unstable `useAsync()`
hook dependency. Every re-render `useAsync()` resolves a promise causing
an additional re-render to show updated results and the cycle repeats.
Such hook is used in
`x-pack/plugins/security_solution/public/common/components/visualization_actions/actions.tsx`\r\n\r\n```ts\r\n
const panels = useAsync(\r\n () =>\r\n buildContextMenuForActions({\r\n
actions: contextMenuActions.map((action) => ({\r\n action,\r\n context:
{},\r\n trigger: VISUALIZATION_CONTEXT_MENU_TRIGGER,\r\n })),\r\n
}),\r\n [contextMenuActions]\r\n );\r\n```\r\n\r\nwhere
`contextMenuActions` is an unstable dependency. This is the case due to
refactoring to `useSaveToLibrary()` hook by **[Security Solution] Remove
usage of deprecated React rendering utilities
[#181099](#181099 which started
retuning a new object every render. The dependency chain is
`contextMenuActions` -> `useActions()` ->
`useSaveToLibrary()`.\r\n\r\nThe actual fix is to
replace\r\n\r\n```ts\r\nconst { lens, ...startServices } =
useKibana().services;\r\n```\r\n\r\nwith\r\n\r\n```ts\r\nconst
startServices = useKibana().services;\r\n```\r\n\r\nSince
`startServices` is used as a hook dependency it must be stable. A rest
property in object destruction expression is always a new object and
can't be used as a dependency as is. Using stable `useKibana().services`
fixes the
problem.","sha":"8a539a8a4ce483d2e5d3aa484d8ec78887f338b8"}}]}]
BACKPORT-->

Co-authored-by: Maxim Palenov <[email protected]>
  • Loading branch information
kibanamachine and maximpn authored Jul 15, 2024
1 parent 3baf161 commit 88524f2
Show file tree
Hide file tree
Showing 7 changed files with 192 additions and 40 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import React, { useCallback, useMemo } from 'react';
import { toMountPoint } from '@kbn/react-kibana-mount';
import type { LensEmbeddableInput } from '@kbn/lens-plugin/public';
import { unmountComponentAtNode } from 'react-dom';
import type { SaveProps } from '@kbn/lens-plugin/public/plugin';
import { useKibana } from '../../lib/kibana';
import type { LensAttributes } from './types';
import { useRedirectToDashboardFromLens } from './use_redirect_to_dashboard_from_lens';
Expand All @@ -21,8 +20,8 @@ export const useSaveToLibrary = ({
}: {
attributes: LensAttributes | undefined | null;
}) => {
const { lens, ...startServices } = useKibana().services;
const { SaveModalComponent, canUseEditor } = lens;
const startServices = useKibana().services;
const { SaveModalComponent, canUseEditor } = startServices.lens;
const getSecuritySolutionUrl = useGetSecuritySolutionUrl();
const { redirectTo, getEditOrCreateDashboardPath } = useRedirectToDashboardFromLens({
getSecuritySolutionUrl,
Expand All @@ -33,12 +32,8 @@ export const useSaveToLibrary = ({
const mount = toMountPoint(
<SaveModalComponent
initialInput={attributes as unknown as LensEmbeddableInput}
onSave={(saveProps: SaveProps) => {
unmountComponentAtNode(targetDomElement);
}}
onClose={() => {
unmountComponentAtNode(targetDomElement);
}}
onSave={() => unmountComponentAtNode(targetDomElement)}
onClose={() => unmountComponentAtNode(targetDomElement)}
originatingApp={APP_UI_ID}
getOriginatingPath={(dashboardId) =>
`${SecurityPageName.dashboards}/${getEditOrCreateDashboardPath(dashboardId)}`
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* 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 { createRule } from '../../../../tasks/api_calls/rules';
import { ruleFields } from '../../../../data/detection_engine';
import { getExistingRule, getNewRule } from '../../../../objects/rule';

import { RULE_NAME_HEADER, RULE_SWITCH } from '../../../../screens/rule_details';

import { createTimeline } from '../../../../tasks/api_calls/timelines';
import { deleteAlertsAndRules, deleteConnectors } from '../../../../tasks/api_calls/common';
import { login } from '../../../../tasks/login';
import { activateSpace, getSpaceUrl } from '../../../../tasks/space';
import { visit } from '../../../../tasks/navigation';
import { ruleDetailsUrl } from '../../../../urls/rule_details';

describe('Non-default space rule detail page', { tags: ['@ess'] }, function () {
const SPACE_ID = 'test';

beforeEach(() => {
login();
activateSpace(SPACE_ID);
deleteAlertsAndRules();
deleteConnectors();
createTimeline().then((response) => {
createRule({
...getNewRule({
rule_id: 'rulez',
description: ruleFields.ruleDescription,
name: ruleFields.ruleName,
severity: ruleFields.ruleSeverity,
risk_score: ruleFields.riskScore,
tags: ruleFields.ruleTags,
false_positives: ruleFields.falsePositives,
note: ruleFields.investigationGuide,
timeline_id: response.body.data.persistTimeline.timeline.savedObjectId,
timeline_title: response.body.data.persistTimeline.timeline.title ?? '',
interval: ruleFields.ruleInterval,
from: `now-1h`,
query: ruleFields.ruleQuery,
enabled: false,
max_signals: 500,
threat: [
{
...ruleFields.threat,
technique: [
{
...ruleFields.threatTechnique,
subtechnique: [ruleFields.threatSubtechnique],
},
],
},
],
}),
}).then((rule) => {
cy.wrap(rule.body.id).as('ruleId');
});
});
});

it('Check responsiveness by enabling/disabling the rule', function () {
visit(getSpaceUrl(SPACE_ID, ruleDetailsUrl(this.ruleId)));
cy.get(RULE_NAME_HEADER).should('contain', ruleFields.ruleName);

cy.intercept(
'POST',
getSpaceUrl(SPACE_ID, '/api/detection_engine/rules/_bulk_action?dry_run=false')
).as('bulk_action');
cy.get(RULE_SWITCH).should('be.visible');
cy.get(RULE_SWITCH).click();
cy.wait('@bulk_action').then(({ response }) => {
cy.wrap(response?.statusCode).should('eql', 200);
cy.wrap(response?.body.attributes.results.updated[0].max_signals).should(
'eql',
getExistingRule().max_signals
);
cy.wrap(response?.body.attributes.results.updated[0].enabled).should('eql', true);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -84,3 +84,8 @@ const waitUntil = (subject, fn, options = {}) => {
};

Cypress.Commands.add('waitUntil', { prevSubject: 'optional' }, waitUntil);

// Sets non-default space id
Cypress.Commands.add('setCurrentSpace', (spaceId) => cy.state('currentSpaceId', spaceId));
// Reads non-default space id
Cypress.Commands.add('currentSpace', () => cy.state('currentSpaceId'));
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@ declare namespace Cypress {
timeout: number;
}
): Chainable<Subject>;
/**
* Sets a new non-default space id as current
*/
setCurrentSpace(spaceId: string): void;
/**
* Reads current space id value. `undefined` is returned for default space.
*/
currentSpace(): Chainable<string>;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@
import { DATA_VIEW_PATH, INITIAL_REST_VERSION } from '@kbn/data-views-plugin/server/constants';
import { ELASTIC_HTTP_VERSION_HEADER } from '@kbn/core-http-common';
import { AllConnectorsResponse } from '@kbn/actions-plugin/common/routes/connector/response';
import { DETECTION_ENGINE_RULES_BULK_ACTION } from '@kbn/security-solution-plugin/common/constants';
import { ELASTICSEARCH_PASSWORD, ELASTICSEARCH_USERNAME } from '../../env_var_names_constants';
import { deleteAllDocuments } from './elasticsearch';
import { DEFAULT_ALERTS_INDEX_PATTERN } from './alerts';
import { getSpaceUrl } from '../space';

export const API_AUTH = Object.freeze({
user: Cypress.env(ELASTICSEARCH_USERNAME),
Expand Down Expand Up @@ -41,18 +43,24 @@ export const rootRequest = <T = unknown>({
export const deleteAlertsAndRules = () => {
cy.log('Delete all alerts and rules');

rootRequest({
method: 'POST',
url: '/api/detection_engine/rules/_bulk_action',
body: {
query: '',
action: 'delete',
},
failOnStatusCode: false,
timeout: 300000,
});
cy.currentSpace().then((spaceId) => {
const url = spaceId
? `/s/${spaceId}${DETECTION_ENGINE_RULES_BULK_ACTION}`
: DETECTION_ENGINE_RULES_BULK_ACTION;

rootRequest({
method: 'POST',
url,
body: {
query: '',
action: 'delete',
},
failOnStatusCode: false,
timeout: 300000,
});

deleteAllDocuments(`.lists-*,.items-*,${DEFAULT_ALERTS_INDEX_PATTERN}`);
deleteAllDocuments(`.lists-*,.items-*,${DEFAULT_ALERTS_INDEX_PATTERN}`);
});
};

export const getConnectors = () =>
Expand All @@ -62,20 +70,24 @@ export const getConnectors = () =>
});

export const deleteConnectors = () => {
getConnectors().then(($response) => {
if ($response.body.length > 0) {
const ids = $response.body.map((connector) => {
return connector.id;
});
ids.forEach((id) => {
if (!INTERNAL_CLOUD_CONNECTORS.includes(id)) {
rootRequest({
method: 'DELETE',
url: `api/actions/connector/${id}`,
});
}
});
}
cy.currentSpace().then((spaceId) => {
getConnectors().then(($response) => {
if ($response.body.length > 0) {
const ids = $response.body.map((connector) => {
return connector.id;
});
ids.forEach((id) => {
if (!INTERNAL_CLOUD_CONNECTORS.includes(id)) {
rootRequest({
method: 'DELETE',
url: spaceId
? getSpaceUrl(spaceId, `api/actions/connector/${id}`)
: `api/actions/connector/${id}`,
});
}
});
}
});
});
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import type {
import type { FetchRulesResponse } from '@kbn/security-solution-plugin/public/detection_engine/rule_management/logic/types';
import { internalAlertingSnoozeRule } from '../../urls/routes';
import { rootRequest } from './common';
import { getSpaceUrl } from '../space';

export const findAllRules = () => {
return rootRequest<FetchRulesResponse>({
Expand All @@ -28,12 +29,14 @@ export const findAllRules = () => {
export const createRule = (
rule: RuleCreateProps
): Cypress.Chainable<Cypress.Response<RuleResponse>> => {
return rootRequest<RuleResponse>({
method: 'POST',
url: DETECTION_ENGINE_RULES_URL,
body: rule,
failOnStatusCode: false,
});
return cy.currentSpace().then((spaceId) =>
rootRequest<RuleResponse>({
method: 'POST',
url: spaceId ? getSpaceUrl(spaceId, DETECTION_ENGINE_RULES_URL) : DETECTION_ENGINE_RULES_URL,
body: rule,
failOnStatusCode: false,
})
);
};

/**
Expand Down
45 changes: 45 additions & 0 deletions x-pack/test/security_solution_cypress/cypress/tasks/space.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* 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 { API_HEADERS } from './api_calls/common';

/**
* Creates a space and sets it as the current one
*/
export function activateSpace(spaceId: string): void {
const baseUrl = Cypress.config().baseUrl;
if (!baseUrl) {
throw Error(`Cypress config baseUrl not set!`);
}

cy.request({
url: `${baseUrl}/api/spaces/space`,
method: 'POST',
body: {
id: spaceId,
name: spaceId,
},
headers: API_HEADERS,
// For the majority cases the specified space already exists and
// this request would fail. To avoid condition logic and an extra
// request to check for space existence it fails silently.
//
// While it will make errors less transparent when a user doesn't
// have credentials to create spaces. But it's a trade off for now
// choosing simplicity over errors transparency.
failOnStatusCode: false,
});

cy.setCurrentSpace(spaceId);
}

/**
* Constructs a space aware url
*/
export function getSpaceUrl(spaceId: string, url: string): string {
return spaceId ? `/s/${spaceId}${url}` : url;
}

0 comments on commit 88524f2

Please sign in to comment.