Skip to content

Commit

Permalink
[Security Solution] [Detections] Fixes validation on response from fi…
Browse files Browse the repository at this point in the history
…nd status route (#93684) (#93857)

* fix validation on response of find status route when rule has a partial failure status

* replaces warning in rule status service with partial failure to maintain backwards compatibility from an API standpoint, also displays 'warning' on UI if a rule's status is partial failure

* display partial failure as 'warning' on all rules table and update e2e test to check for partial failure not warning

* add util function, show 'warning' on monitoring table, fix e2e tests
# Conflicts:
#	x-pack/plugins/security_solution/common/detection_engine/utils.ts
#	x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx
#	x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.test.ts
#	x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts
#	x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules.ts
  • Loading branch information
dhurley14 authored Mar 6, 2021
1 parent 2a5c313 commit bdba929
Show file tree
Hide file tree
Showing 10 changed files with 40 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ export const job_status = t.keyof({
succeeded: null,
failed: null,
'going to run': null,
'partial failure': null,
warning: null,
});
export type JobStatus = t.TypeOf<typeof job_status>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/

import { EntriesArray } from '../shared_imports';
import { Type } from './schemas/common/schemas';
import { Type, JobStatus } from './schemas/common/schemas';

export const hasLargeValueList = (entries: EntriesArray): boolean => {
const found = entries.filter(({ type }) => type === 'list');
Expand All @@ -31,3 +31,6 @@ export const isThresholdRule = (ruleType: Type | undefined): boolean => ruleType
export const isQueryRule = (ruleType: Type | undefined): boolean =>
ruleType === 'query' || ruleType === 'saved_query';
export const isThreatMatchRule = (ruleType: Type): boolean => ruleType === 'threat_match';

export const getRuleStatusText = (value: JobStatus | null | undefined): JobStatus | null =>
value === 'partial failure' ? 'warning' : value != null ? value : null;
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,6 @@ export interface RuleStatus {
}

export type RuleStatusType =
| 'executing'
| 'failed'
| 'going to run'
| 'succeeded'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import { LocalizedDateTooltip } from '../../../../../common/components/localized
import { LinkAnchor } from '../../../../../common/components/links';
import { getToolTipContent, canEditRuleWithActions } from '../../../../../common/utils/privileges';
import { TagsDisplay } from './tag_display';
import { getRuleStatusText } from '../../../../../../common/detection_engine/utils';

export const getActions = (
dispatch: React.Dispatch<Action>,
Expand Down Expand Up @@ -195,7 +196,7 @@ export const getColumns = ({
return (
<>
<EuiHealth color={getStatusColor(value ?? null)}>
{value ?? getEmptyTagValue()}
{getRuleStatusText(value) ?? getEmptyTagValue()}
</EuiHealth>
</>
);
Expand Down Expand Up @@ -391,7 +392,7 @@ export const getMonitoringColumns = (
return (
<>
<EuiHealth color={getStatusColor(value ?? null)}>
{value ?? getEmptyTagValue()}
{getRuleStatusText(value) ?? getEmptyTagValue()}
</EuiHealth>
</>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export const ruleStatusServiceFactoryMock = async ({

success: jest.fn(),

warning: jest.fn(),
partialFailure: jest.fn(),

error: jest.fn(),
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ interface Attributes {
export interface RuleStatusService {
goingToRun: () => Promise<void>;
success: (message: string, attributes?: Attributes) => Promise<void>;
warning: (message: string, attributes?: Attributes) => Promise<void>;
partialFailure: (message: string, attributes?: Attributes) => Promise<void>;
error: (message: string, attributes?: Attributes) => Promise<void>;
}

Expand Down Expand Up @@ -55,6 +55,13 @@ export const buildRuleStatusAttributes: (
lastSuccessMessage: message,
};
}
case 'partial failure': {
return {
...baseAttributes,
lastSuccessAt: now,
lastSuccessMessage: message,
};
}
case 'failed': {
return {
...baseAttributes,
Expand Down Expand Up @@ -102,15 +109,15 @@ export const ruleStatusServiceFactory = async ({
});
},

warning: async (message, attributes) => {
partialFailure: async (message, attributes) => {
const [currentStatus] = await getOrCreateRuleStatuses({
alertId,
ruleStatusClient,
});

await ruleStatusClient.update(currentStatus.id, {
...currentStatus.attributes,
...buildRuleStatusAttributes('warning', message, attributes),
...buildRuleStatusAttributes('partial failure', message, attributes),
});
},

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ describe('rules_notification_alert_type', () => {
find: jest.fn(),
goingToRun: jest.fn(),
error: jest.fn(),
warning: jest.fn(),
partialFailure: jest.fn(),
};
(ruleStatusServiceFactory as jest.Mock).mockReturnValue(ruleStatusService);
(getGapBetweenRuns as jest.Mock).mockReturnValue(moment.duration(0));
Expand Down Expand Up @@ -223,8 +223,8 @@ describe('rules_notification_alert_type', () => {
});
payload.params.index = ['some*', 'myfa*', 'anotherindex*'];
await alert.executor(payload);
expect(ruleStatusService.warning).toHaveBeenCalled();
expect(ruleStatusService.warning.mock.calls[0][0]).toContain(
expect(ruleStatusService.partialFailure).toHaveBeenCalled();
expect(ruleStatusService.partialFailure.mock.calls[0][0]).toContain(
'Missing required read privileges on the following indices: ["some*"]'
);
});
Expand All @@ -246,8 +246,8 @@ describe('rules_notification_alert_type', () => {
});
payload.params.index = ['some*', 'myfa*'];
await alert.executor(payload);
expect(ruleStatusService.warning).toHaveBeenCalled();
expect(ruleStatusService.warning.mock.calls[0][0]).toContain(
expect(ruleStatusService.partialFailure).toHaveBeenCalled();
expect(ruleStatusService.partialFailure.mock.calls[0][0]).toContain(
'This rule may not have the required read privileges to the following indices: ["myfa*","some*"]'
);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ const ruleStatusServiceMock = {
find: jest.fn(),
goingToRun: jest.fn(),
error: jest.fn(),
warning: jest.fn(),
partialFailure: jest.fn(),
};

describe('utils', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ export const hasReadIndexPrivileges = async (
indexesWithNoReadPrivileges
)}`;
logger.error(buildRuleMessage(errorString));
await ruleStatusService.warning(errorString);
await ruleStatusService.partialFailure(errorString);
return true;
} else if (
indexesWithReadPrivileges.length === 0 &&
Expand All @@ -91,7 +91,7 @@ export const hasReadIndexPrivileges = async (
indexesWithNoReadPrivileges
)}`;
logger.error(buildRuleMessage(errorString));
await ruleStatusService.warning(errorString);
await ruleStatusService.partialFailure(errorString);
return true;
}
return false;
Expand Down Expand Up @@ -119,7 +119,7 @@ export const hasTimestampFields = async (
: ''
}`;
logger.error(buildRuleMessage(errorString.trimEnd()));
await ruleStatusService.warning(errorString.trimEnd());
await ruleStatusService.partialFailure(errorString.trimEnd());
return true;
} else if (
!wroteStatus &&
Expand All @@ -140,7 +140,7 @@ export const hasTimestampFields = async (
: timestampFieldCapsResponse.body.fields[timestampField]?.unmapped?.indices
)}`;
logger.error(buildRuleMessage(errorString));
await ruleStatusService.warning(errorString);
await ruleStatusService.partialFailure(errorString);
return true;
}
return wroteStatus;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,23 +116,23 @@ export default ({ getService }: FtrProviderContext) => {
expect(statusBody[body.id].current_status.status).to.eql('succeeded');
});

it('should create a single rule with a rule_id and an index pattern that does not match anything available and warning for the rule', async () => {
it('should create a single rule with a rule_id and an index pattern that does not match anything available and partial failure for the rule', async () => {
const simpleRule = getRuleForSignalTesting(['does-not-exist-*']);
const { body } = await supertest
.post(DETECTION_ENGINE_RULES_URL)
.set('kbn-xsrf', 'true')
.send(simpleRule)
.expect(200);

await waitForRuleSuccessOrStatus(supertest, body.id, 'warning');
await waitForRuleSuccessOrStatus(supertest, body.id, 'partial failure');

const { body: statusBody } = await supertest
.post(DETECTION_ENGINE_RULES_STATUS_URL)
.set('kbn-xsrf', 'true')
.send({ ids: [body.id] })
.expect(200);

expect(statusBody[body.id].current_status.status).to.eql('warning');
expect(statusBody[body.id].current_status.status).to.eql('partial failure');
expect(statusBody[body.id].current_status.last_success_message).to.eql(
'This rule is attempting to query data from Elasticsearch indices listed in the "Index pattern" section of the rule definition, however no index matching: ["does-not-exist-*"] was found. This warning will continue to appear until a matching index is created or this rule is de-activated.'
);
Expand Down Expand Up @@ -264,7 +264,7 @@ export default ({ getService }: FtrProviderContext) => {
await esArchiver.unload('security_solution/timestamp_override');
});

it('should create a single rule which has a timestamp override for an index pattern that does not exist and write a warning status', async () => {
it('should create a single rule which has a timestamp override for an index pattern that does not exist and write a partial failure status', async () => {
// defaults to event.ingested timestamp override.
// event.ingested is one of the timestamp fields set on the es archive data
// inside of x-pack/test/functional/es_archives/security_solution/timestamp_override/data.json.gz
Expand All @@ -277,23 +277,25 @@ export default ({ getService }: FtrProviderContext) => {
const bodyId = body.id;

await waitForAlertToComplete(supertest, bodyId);
await waitForRuleSuccessOrStatus(supertest, bodyId, 'warning');
await waitForRuleSuccessOrStatus(supertest, bodyId, 'partial failure');

const { body: statusBody } = await supertest
.post(DETECTION_ENGINE_RULES_STATUS_URL)
.set('kbn-xsrf', 'true')
.send({ ids: [bodyId] })
.expect(200);

expect((statusBody as RuleStatusResponse)[bodyId].current_status?.status).to.eql('warning');
expect((statusBody as RuleStatusResponse)[bodyId].current_status?.status).to.eql(
'partial failure'
);
expect(
(statusBody as RuleStatusResponse)[bodyId].current_status?.last_success_message
).to.eql(
'The following indices are missing the timestamp override field "event.ingested": ["myfakeindex-1"]'
);
});

it('should create a single rule which has a timestamp override and generates two signals with a "warning" status', async () => {
it('should create a single rule which has a timestamp override and generates two signals with a "partial failure" status', async () => {
// defaults to event.ingested timestamp override.
// event.ingested is one of the timestamp fields set on the es archive data
// inside of x-pack/test/functional/es_archives/security_solution/timestamp_override/data.json.gz
Expand All @@ -305,7 +307,7 @@ export default ({ getService }: FtrProviderContext) => {
.expect(200);
const bodyId = body.id;

await waitForRuleSuccessOrStatus(supertest, bodyId, 'warning');
await waitForRuleSuccessOrStatus(supertest, bodyId, 'partial failure');
await waitForSignalsToBePresent(supertest, 2, [bodyId]);

const { body: statusBody } = await supertest
Expand All @@ -314,7 +316,7 @@ export default ({ getService }: FtrProviderContext) => {
.send({ ids: [bodyId] })
.expect(200);

expect(statusBody[bodyId].current_status.status).to.eql('warning');
expect(statusBody[bodyId].current_status.status).to.eql('partial failure');
});
});
});
Expand Down

0 comments on commit bdba929

Please sign in to comment.