From 0bfe68fbc2c193afb97b91ed2560d3e1e7526302 Mon Sep 17 00:00:00 2001 From: Pablo Machado Date: Thu, 6 Mar 2025 10:51:08 +0100 Subject: [PATCH] [SecuritySolution] Fix Risk score Insufficient privileges warning missing cluster privileges (#212405) ## Summary ### * Fixes Bug: User with no cluster privileges should not be able to enable the risk score When users with no cluster privileges open the risk score page, they don't see any errors and are able to click the install button. This happened because we were only checking for index privileges in the UI, but for the enablement flow we also need to check cluster privileges. I also introduced a new parameter to the missing privileges hook so pages that only need to check for `read` privileges can work as before. https://github.com/user-attachments/assets/fe162005-ee2b-497d-8744-6262e4511d2d * Fixed Bug: The install button was enabled when all toggles were disabled There were too many booleans in the panel, which was confusing and led me to introduce more bugs while trying to fix this one, so I refactored the code to understand it before fixing it. I also simplified the logic to display the modal. Now, it only shows when one of the engines' status is "not_installed" ### To Reproduce 1. Create a user with security privileges and index privileges but no cluster privileges 2. Go to the risk score page and enable the toggle ### Checklist Check the PR satisfies following conditions. - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or (cherry picked from commit b69b696e7fddf4bb26d038f1deaa6388051c428d) # Conflicts: # x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/entity_analytics_risk_score/index.tsx # x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/risk_details_tab_body/index.tsx # x-pack/solutions/security/plugins/security_solution/public/explore/hosts/pages/navigation/host_risk_score_tab_body.tsx --- .../entity_analytics_risk_score/index.tsx | 2 +- .../components/dashboard_enablement_panel.tsx | 12 +- .../components/enablement_modal.test.tsx | 52 ++++---- .../components/enablement_modal.tsx | 113 ++++++++++-------- .../risk_details_tab_body/index.tsx | 2 +- .../components/user_risk_score_tab_body.tsx | 2 +- .../use_missing_risk_engine_privileges.ts | 23 +++- .../navigation/host_risk_score_tab_body.tsx | 2 +- 8 files changed, 122 insertions(+), 86 deletions(-) diff --git a/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/entity_analytics_risk_score/index.tsx b/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/entity_analytics_risk_score/index.tsx index 9695e5c318580..ebfca392af255 100644 --- a/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/entity_analytics_risk_score/index.tsx +++ b/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/entity_analytics_risk_score/index.tsx @@ -176,7 +176,7 @@ const EntityAnalyticsRiskScoresComponent = ({ const refreshPage = useRefetchQueries(); - const privileges = useMissingRiskEnginePrivileges(['read']); + const privileges = useMissingRiskEnginePrivileges({ readonly: true }); if (!isAuthorized) { return null; diff --git a/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/entity_store/components/dashboard_enablement_panel.tsx b/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/entity_store/components/dashboard_enablement_panel.tsx index 984c00fac579b..d8a8fdb300a69 100644 --- a/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/entity_store/components/dashboard_enablement_panel.tsx +++ b/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/entity_store/components/dashboard_enablement_panel.tsx @@ -185,7 +185,7 @@ export const EnablementPanel: React.FC = ({ state } if ( riskEngineStatus !== RiskEngineStatusEnum.NOT_INSTALLED && - (entityStoreStatus === 'running' || entityStoreStatus === 'stopped') + entityStoreStatus !== 'not_installed' ) { return null; } @@ -222,14 +222,8 @@ export const EnablementPanel: React.FC = ({ state } visible={modal.visible} toggle={(visible) => setModalState({ visible })} enableStore={enableEntityStore} - riskScore={{ - canToggle: riskEngineStatus === RiskEngineStatusEnum.NOT_INSTALLED, - checked: riskEngineStatus === RiskEngineStatusEnum.NOT_INSTALLED, - }} - entityStore={{ - canToggle: entityStoreStatus !== 'running', - checked: entityStoreStatus === 'not_installed', - }} + riskEngineStatus={riskEngineStatus} + entityStoreStatus={entityStoreStatus} /> ); diff --git a/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/entity_store/components/enablement_modal.test.tsx b/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/entity_store/components/enablement_modal.test.tsx index 80a16e18ef6c8..4537fdb81dceb 100644 --- a/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/entity_store/components/enablement_modal.test.tsx +++ b/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/entity_store/components/enablement_modal.test.tsx @@ -8,9 +8,14 @@ import React from 'react'; import { act } from 'react-dom/test-utils'; import { fireEvent, render, screen } from '@testing-library/react'; +import type { EntityStoreEnablementModalProps } from './enablement_modal'; import { EntityStoreEnablementModal } from './enablement_modal'; import { TestProviders } from '../../../../common/mock'; -import type { EntityAnalyticsPrivileges } from '../../../../../common/api/entity_analytics'; +import { + type EntityAnalyticsPrivileges, + RiskEngineStatusEnum, + StoreStatusEnum, +} from '../../../../../common/api/entity_analytics'; import type { RiskEngineMissingPrivilegesResponse } from '../../../hooks/use_missing_risk_engine_privileges'; const mockToggle = jest.fn(); @@ -35,8 +40,8 @@ const defaultProps = { visible: true, toggle: mockToggle, enableStore: mockEnableStore, - riskScore: { canToggle: false, checked: false }, - entityStore: { canToggle: false, checked: false }, + riskEngineStatus: RiskEngineStatusEnum.NOT_INSTALLED, + entityStoreStatus: StoreStatusEnum.not_installed, }; const allEntityEnginePrivileges: EntityAnalyticsPrivileges = { @@ -83,7 +88,7 @@ const missingRiskEnginePrivileges: RiskEngineMissingPrivilegesResponse = { }, }; -const renderComponent = async (props = defaultProps) => { +const renderComponent = async (props: EntityStoreEnablementModalProps = defaultProps) => { await act(async () => { return render(, { wrapper: TestProviders }); }); @@ -121,46 +126,45 @@ describe('EntityStoreEnablementModal', () => { }); it('should call enableStore function when enable button is clicked', () => { - renderComponent({ - ...defaultProps, - riskScore: { ...defaultProps.riskScore, checked: true }, - entityStore: { ...defaultProps.entityStore, checked: true }, - }); + renderComponent(defaultProps); fireEvent.click(screen.getByText('Enable')); expect(mockEnableStore).toHaveBeenCalledWith({ riskScore: true, entityStore: true }); }); it('should display proceed warning when no enablement options are selected', () => { renderComponent(); + fireEvent.click(screen.getByTestId('enablementEntityStoreSwitch')); // unselect entity store + fireEvent.click(screen.getByTestId('enablementRiskScoreSwitch')); // unselect risk engine expect(screen.getByText('Please enable at least one option to proceed.')).toBeInTheDocument(); }); it('should disable the enable button when enablementOptions are false', () => { - renderComponent({ - ...defaultProps, - riskScore: { ...defaultProps.riskScore, checked: false }, - entityStore: { ...defaultProps.entityStore, checked: false }, - }); + renderComponent(); + fireEvent.click(screen.getByTestId('enablementEntityStoreSwitch')); // unselect entity store + fireEvent.click(screen.getByTestId('enablementRiskScoreSwitch')); // unselect risk engine const enableButton = screen.getByRole('button', { name: /Enable/i }); expect(enableButton).toBeDisabled(); }); - it('should show proceed warning when riskScore is enabled but entityStore is disabled and unchecked', () => { + it('should show proceed warning when riskScore is not installed and unchecked but entityStore is already running', () => { renderComponent({ ...defaultProps, - riskScore: { canToggle: false, checked: false }, // Enabled & Checked - entityStore: { canToggle: true, checked: false }, // Disabled & Unchecked + riskEngineStatus: RiskEngineStatusEnum.NOT_INSTALLED, + entityStoreStatus: StoreStatusEnum.running, }); + fireEvent.click(screen.getByTestId('enablementRiskScoreSwitch')); // unselect risk engine expect(screen.getByText('Please enable at least one option to proceed.')).toBeInTheDocument(); }); - it('should show proceed warning when entityStore is enabled but riskScore is disabled and unchecked', () => { + it('should show proceed warning when entityStore is not installed and unchecked but riskScore is already installed', () => { renderComponent({ ...defaultProps, - entityStore: { canToggle: false, checked: false }, // Enabled & Checked - riskScore: { canToggle: true, checked: false }, // Disabled & Unchecked + riskEngineStatus: RiskEngineStatusEnum.ENABLED, + entityStoreStatus: StoreStatusEnum.not_installed, }); + fireEvent.click(screen.getByTestId('enablementEntityStoreSwitch')); // unselect risk engine + expect(screen.getByText('Please enable at least one option to proceed.')).toBeInTheDocument(); }); @@ -209,5 +213,13 @@ describe('EntityStoreEnablementModal', () => { expect(screen.queryByTestId('enablement-modal-test')).toBeInTheDocument(); }); + + it('should disabled the "enable" button', async () => { + renderComponent(); + expect(screen.getByTestId('callout-missing-entity-store-privileges')).toBeInTheDocument(); + + const enableButton = screen.getByRole('button', { name: /Enable/i }); + expect(enableButton).toBeDisabled(); + }); }); }); diff --git a/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/entity_store/components/enablement_modal.tsx b/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/entity_store/components/enablement_modal.tsx index 9347273ce5590..61ab9ba53edca 100644 --- a/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/entity_store/components/enablement_modal.tsx +++ b/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/components/entity_store/components/enablement_modal.tsx @@ -22,73 +22,94 @@ import { useEuiTheme, } from '@elastic/eui'; import { css } from '@emotion/react'; -import React, { useState } from 'react'; +import React, { useEffect, useState } from 'react'; import { FormattedMessage } from '@kbn/i18n-react'; +import type { RiskEngineStatus, StoreStatus } from '../../../../../common/api/entity_analytics'; +import { RiskEngineStatusEnum } from '../../../../../common/api/entity_analytics'; import { useContractComponents } from '../../../../common/hooks/use_contract_component'; import { ENABLEMENT_DESCRIPTION_RISK_ENGINE_ONLY, ENABLEMENT_DESCRIPTION_ENTITY_STORE_ONLY, ENABLEMENT_WARNING_SELECT_TO_PROCEED, } from '../translations'; -import { useEntityEnginePrivileges } from '../hooks/use_entity_engine_privileges'; import { MissingPrivilegesCallout } from './missing_privileges_callout'; import { useMissingRiskEnginePrivileges } from '../../../hooks/use_missing_risk_engine_privileges'; import { RiskEnginePrivilegesCallOut } from '../../risk_engine_privileges_callout'; +import { useEntityEnginePrivileges } from '../hooks/use_entity_engine_privileges'; export interface Enablements { riskScore: boolean; entityStore: boolean; } -interface EntityStoreEnablementModalProps { +export interface EntityStoreEnablementModalProps { visible: boolean; toggle: (visible: boolean) => void; enableStore: (enablements: Enablements) => () => void; - riskScore: { - canToggle?: boolean; - checked?: boolean; - }; - entityStore: { - canToggle?: boolean; - checked?: boolean; - }; + riskEngineStatus?: RiskEngineStatus; + entityStoreStatus?: StoreStatus; } -const shouldAllowEnablement = ( - riskScoreEnabled: boolean, - entityStoreEnabled: boolean, +const isInstallButtonEnabled = ( + canInstallRiskScore: boolean, + canInstallEntityStore: boolean, userHasEnabled: Enablements ) => { - if (riskScoreEnabled) { - return userHasEnabled.entityStore; - } - if (entityStoreEnabled) { - return userHasEnabled.riskScore; + if (canInstallRiskScore || canInstallEntityStore) { + return userHasEnabled.riskScore || userHasEnabled.entityStore; } - return userHasEnabled.riskScore || userHasEnabled.entityStore; + + return false; }; export const EntityStoreEnablementModal: React.FC = ({ visible, toggle, enableStore, - riskScore, - entityStore, + riskEngineStatus, + entityStoreStatus, }) => { - const { euiTheme } = useEuiTheme(); - const [enablements, setEnablements] = useState({ - riskScore: !!riskScore.checked, - entityStore: !!entityStore.checked, - }); + const riskEnginePrivileges = useMissingRiskEnginePrivileges(); const { data: entityEnginePrivileges, isLoading: isLoadingEntityEnginePrivileges } = useEntityEnginePrivileges(); - const riskEnginePrivileges = useMissingRiskEnginePrivileges(); - const enablementOptions = shouldAllowEnablement( - !riskScore.canToggle, - !entityStore.canToggle, - enablements + const hasRiskScorePrivileges = !( + riskEnginePrivileges.isLoading || !riskEnginePrivileges?.hasAllRequiredPrivileges + ); + + const canInstallRiskScore = + hasRiskScorePrivileges && riskEngineStatus === RiskEngineStatusEnum.NOT_INSTALLED; + + const hasEntityStorePrivileges = !( + isLoadingEntityEnginePrivileges || !entityEnginePrivileges?.has_all_required ); + + const canInstallEntityStore = hasEntityStorePrivileges && entityStoreStatus === 'not_installed'; + + const { euiTheme } = useEuiTheme(); + const [toggleState, setToggleState] = useState({ + riskScore: false, + entityStore: false, + }); + + /** + * Update the toggle state when the install status changes because privileges are async. + * We automatically toggle the switch when the user can enable the engine. + * + */ + useEffect(() => { + setToggleState({ + riskScore: canInstallRiskScore, + entityStore: canInstallEntityStore, + }); + }, [canInstallRiskScore, canInstallEntityStore]); + + const isInstallButtonDisabled = !isInstallButtonEnabled( + canInstallRiskScore, + canInstallEntityStore, + toggleState + ); + const { AdditionalChargesMessage } = useContractComponents(); if (!visible) { @@ -127,12 +148,9 @@ export const EntityStoreEnablementModal: React.FC } - checked={enablements.riskScore} - disabled={ - !riskScore.canToggle || - (!riskEnginePrivileges.isLoading && !riskEnginePrivileges?.hasAllRequiredPrivileges) - } - onChange={() => setEnablements((prev) => ({ ...prev, riskScore: !prev.riskScore }))} + checked={toggleState.riskScore} + disabled={!canInstallRiskScore} + onChange={() => setToggleState((prev) => ({ ...prev, riskScore: !prev.riskScore }))} data-test-subj="enablementRiskScoreSwitch" /> @@ -154,13 +172,10 @@ export const EntityStoreEnablementModal: React.FC } - checked={enablements.entityStore} - disabled={ - !entityStore.canToggle || - (!isLoadingEntityEnginePrivileges && !entityEnginePrivileges?.has_all_required) - } + checked={toggleState.entityStore} + disabled={!canInstallEntityStore} onChange={() => - setEnablements((prev) => ({ ...prev, entityStore: !prev.entityStore })) + setToggleState((prev) => ({ ...prev, entityStore: !prev.entityStore })) } data-test-subj="enablementEntityStoreSwitch" /> @@ -179,15 +194,17 @@ export const EntityStoreEnablementModal: React.FC - {!enablementOptions ? {proceedWarning} : null} + {isInstallButtonDisabled && (canInstallRiskScore || canInstallEntityStore) ? ( + {proceedWarning} + ) : null} toggle(false)}>{'Cancel'} ({ from, to }), [from, to]); - const privileges = useMissingRiskEnginePrivileges(); + const privileges = useMissingRiskEnginePrivileges({ readonly: true }); const { data, diff --git a/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/hooks/use_missing_risk_engine_privileges.ts b/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/hooks/use_missing_risk_engine_privileges.ts index 9fa4c8d4b3881..f9a07684bbb9c 100644 --- a/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/hooks/use_missing_risk_engine_privileges.ts +++ b/x-pack/solutions/security/plugins/security_solution/public/entity_analytics/hooks/use_missing_risk_engine_privileges.ts @@ -22,8 +22,14 @@ export type RiskEngineMissingPrivilegesResponse = hasAllRequiredPrivileges: false; }; +interface UseMissingRiskEnginePrivilegesParams { + /** + * If `true`, only read privileges are required. + */ + readonly: boolean; +} export const useMissingRiskEnginePrivileges = ( - required: NonEmptyArray = ['read', 'write'] + { readonly }: UseMissingRiskEnginePrivilegesParams = { readonly: false } ): RiskEngineMissingPrivilegesResponse => { const { data: privilegesResponse, isLoading } = useRiskEnginePrivileges(); @@ -41,14 +47,21 @@ export const useMissingRiskEnginePrivileges = ( }; } + const requiredIndexPrivileges: NonEmptyArray = readonly + ? ['read'] + : ['read', 'write']; + const { indexPrivileges, clusterPrivileges } = getMissingRiskEnginePrivileges( privilegesResponse.privileges, - required + requiredIndexPrivileges ); // privilegesResponse.has_all_required` is slightly misleading, it checks if it has *all* default required privileges. // Here we check if there are no missing privileges of the provided set of required privileges - if (indexPrivileges.every(([_, missingPrivileges]) => missingPrivileges.length === 0)) { + if ( + indexPrivileges.every(([_, missingPrivileges]) => missingPrivileges.length === 0) && + (readonly || clusterPrivileges.length === 0) // cluster privileges check is required for write operations + ) { return { isLoading: false, hasAllRequiredPrivileges: true, @@ -60,8 +73,8 @@ export const useMissingRiskEnginePrivileges = ( hasAllRequiredPrivileges: false, missingPrivileges: { indexPrivileges, - clusterPrivileges, + clusterPrivileges: readonly ? [] : clusterPrivileges, // cluster privileges are not required for readonly }, }; - }, [isLoading, privilegesResponse, required]); + }, [isLoading, privilegesResponse, readonly]); }; diff --git a/x-pack/solutions/security/plugins/security_solution/public/explore/hosts/pages/navigation/host_risk_score_tab_body.tsx b/x-pack/solutions/security/plugins/security_solution/public/explore/hosts/pages/navigation/host_risk_score_tab_body.tsx index 90a2590915b05..c75541a4d0765 100644 --- a/x-pack/solutions/security/plugins/security_solution/public/explore/hosts/pages/navigation/host_risk_score_tab_body.tsx +++ b/x-pack/solutions/security/plugins/security_solution/public/explore/hosts/pages/navigation/host_risk_score_tab_body.tsx @@ -67,7 +67,7 @@ export const HostRiskScoreQueryTabBody = ({ }, [toggleStatus]); const timerange = useMemo(() => ({ from, to }), [from, to]); - const privileges = useMissingRiskEnginePrivileges(); + const privileges = useMissingRiskEnginePrivileges({ readonly: true }); const { data, inspect,