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 (elastic#93684)

* 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
  • Loading branch information
dhurley14 authored and kibanamachine committed Mar 5, 2021
1 parent 80a3764 commit b056e78
Show file tree
Hide file tree
Showing 12 changed files with 51 additions and 34 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 @@ -12,7 +12,7 @@ import {
EntriesArray,
ExceptionListItemSchema,
} from '../shared_imports';
import { Type } from './schemas/common/schemas';
import { Type, JobStatus } from './schemas/common/schemas';

export const hasLargeValueItem = (
exceptionItems: Array<ExceptionListItemSchema | CreateExceptionListItemSchema>
Expand Down Expand Up @@ -54,3 +54,6 @@ export const normalizeThresholdField = (
? []
: [thresholdField!];
};

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 @@ -256,7 +256,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<RulesTableAction>,
Expand Down Expand Up @@ -201,7 +202,7 @@ export const getColumns = ({
return (
<>
<EuiHealth color={getStatusColor(value ?? null)}>
{value ?? getEmptyTagValue()}
{getRuleStatusText(value) ?? getEmptyTagValue()}
</EuiHealth>
</>
);
Expand Down Expand Up @@ -398,7 +399,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 @@ -107,6 +107,7 @@ import * as statusI18n from '../../../../components/rules/rule_status/translatio
import * as i18n from './translations';
import { isTab } from '../../../../../common/components/accessibility/helpers';
import { NeedAdminForUpdateRulesCallOut } from '../../../../components/callouts/need_admin_for_update_callout';
import { getRuleStatusText } from '../../../../../../common/detection_engine/utils';

/**
* Need a 100% height here to account for the graph/analyze tool, which sets no explicit height parameters, but fills the available space.
Expand Down Expand Up @@ -328,7 +329,10 @@ const RuleDetailsPageComponent = () => {
</EuiFlexItem>
) : (
<>
<RuleStatus status={currentStatus?.status} statusDate={currentStatus?.status_date}>
<RuleStatus
status={getRuleStatusText(currentStatus?.status)}
statusDate={currentStatus?.status_date}
>
<EuiButtonIcon
data-test-subj="refreshButton"
color="primary"
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 @@ -107,7 +107,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);
(getListsClient as jest.Mock).mockReturnValue({
Expand Down Expand Up @@ -211,8 +211,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 @@ -223,8 +223,8 @@ describe('rules_notification_alert_type', () => {
]);
payload = getPayload(getThresholdResult(), alertServices);
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(
'Exceptions that use "is in list" or "is not in list" operators are not applied to Threshold rules'
);
});
Expand All @@ -235,8 +235,8 @@ describe('rules_notification_alert_type', () => {
]);
payload = getPayload(getEqlResult(), alertServices);
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(
'Exceptions that use "is in list" or "is not in list" operators are not applied to EQL rules'
);
});
Expand All @@ -258,8 +258,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 @@ -374,7 +374,7 @@ export const signalRulesAlertType = ({
]);
} else if (isThresholdRule(type) && threshold) {
if (hasLargeValueItem(exceptionItems ?? [])) {
await ruleStatusService.warning(
await ruleStatusService.partialFailure(
'Exceptions that use "is in list" or "is not in list" operators are not applied to Threshold rules'
);
wroteWarningStatus = true;
Expand Down Expand Up @@ -577,7 +577,7 @@ export const signalRulesAlertType = ({
throw new Error('EQL query rule must have a query defined');
}
if (hasLargeValueItem(exceptionItems ?? [])) {
await ruleStatusService.warning(
await ruleStatusService.partialFailure(
'Exceptions that use "is in list" or "is not in list" operators are not applied to EQL rules'
);
wroteWarningStatus = true;
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 @@ -84,7 +84,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 @@ -96,7 +96,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 @@ -124,7 +124,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 @@ -145,7 +145,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 @@ -120,23 +120,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 @@ -290,7 +290,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 @@ -303,23 +303,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 @@ -331,7 +333,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 @@ -340,7 +342,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 b056e78

Please sign in to comment.