Skip to content

Commit

Permalink
removes skip from e2e test for duplicates on bulk create, updates exp…
Browse files Browse the repository at this point in the history 
…ected response in e2e test, fixes bug where duplicated error messages appeared for each instance of a duplicated rule_id (found this one through the e2e tests)! Adds unit test to catch this case.
  • Loading branch information
@dhurley14
dhurley14 committed Feb 12, 2020
1 parent b58e9d6 commit e2609c1
Show file tree
Hide file tree
Showing 5 changed files with 111 additions and 88 deletions.
15 changes: 15 additions & 0 deletions ...acy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,4 +141,19 @@ describe('create_rules_bulk', () => {
const output: Array<BulkError | Partial<OutputRuleAlertRest>> = JSON.parse(payload);
expect(output.some(item => item.error?.status_code === 409)).toBeTruthy();
});

test('returns one error object in response when duplicate rule_ids found in request payload', async () => {
alertsClient.find.mockResolvedValue(getFindResult());
alertsClient.get.mockResolvedValue(getResult());
actionsClient.create.mockResolvedValue(createActionResult());
alertsClient.create.mockResolvedValue(getResult());
const request: ServerInjectOptions = {
method: 'POST',
url: `${DETECTION_ENGINE_RULES_URL}/_bulk_create`,
payload: [typicalPayload(), typicalPayload()],
};
const { payload } = await server.inject(request);
const output: Array<BulkError | Partial<OutputRuleAlertRest>> = JSON.parse(payload);
expect(output.length).toBe(1);
});
});
164 changes: 86 additions & 78 deletions ...k/legacy/plugins/siem/server/lib/detection_engine/routes/rules/create_rules_bulk_route.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,104 +48,112 @@ export const createCreateRulesBulkRoute = (server: ServerFacade): Hapi.ServerRou
return headers.response().code(404);
}

const mappedDuplicates = countBy('rule_id', request.payload);
const ruleDefinitions = request.payload;
const mappedDuplicates = countBy('rule_id', ruleDefinitions);
const dupes = getDuplicates(mappedDuplicates);

const rules = await Promise.all(
request.payload.map(async payloadRule => {
const {
description,
enabled,
false_positives: falsePositives,
from,
query,
language,
output_index: outputIndex,
saved_id: savedId,
meta,
filters,
rule_id: ruleId,
index,
interval,
max_signals: maxSignals,
risk_score: riskScore,
name,
severity,
tags,
threat,
to,
type,
references,
timeline_id: timelineId,
timeline_title: timelineTitle,
version,
} = payloadRule;
if (ruleId != null && dupes?.includes(ruleId)) {
return createBulkErrorObject({
ruleId,
statusCode: 409,
message: `rule_id: "${ruleId}" already exists`,
});
}
const ruleIdOrUuid = ruleId ?? uuid.v4();
try {
const finalIndex = outputIndex != null ? outputIndex : getIndex(request, server);
const callWithRequest = callWithRequestFactory(request, server);
const indexExists = await getIndexExists(callWithRequest, finalIndex);
if (!indexExists) {
return createBulkErrorObject({
ruleId: ruleIdOrUuid,
statusCode: 400,
message: `To create a rule, the index must exist first. Index ${finalIndex} does not exist`,
});
}
if (ruleId != null) {
const rule = await readRules({ alertsClient, ruleId });
if (rule != null) {
return createBulkErrorObject({
ruleId,
statusCode: 409,
message: `rule_id: "${ruleId}" already exists`,
});
}
}
const createdRule = await createRules({
alertsClient,
actionsClient,
ruleDefinitions
.filter(rule => !dupes?.includes(rule.rule_id ?? ''))
.map(async payloadRule => {
const {
description,
enabled,
falsePositives,
false_positives: falsePositives,
from,
immutable: false,
query,
language,
outputIndex: finalIndex,
savedId,
timelineId,
timelineTitle,
output_index: outputIndex,
saved_id: savedId,
meta,
filters,
ruleId: ruleIdOrUuid,
rule_id: ruleId,
index,
interval,
maxSignals,
riskScore,
max_signals: maxSignals,
risk_score: riskScore,
name,
severity,
tags,
threat,
to,
type,
threat,
references,
timeline_id: timelineId,
timeline_title: timelineTitle,
version,
});
return transformOrBulkError(ruleIdOrUuid, createdRule);
} catch (err) {
return transformBulkError(ruleIdOrUuid, err);
}
})
} = payloadRule;
const ruleIdOrUuid = ruleId ?? uuid.v4();
try {
const finalIndex = outputIndex != null ? outputIndex : getIndex(request, server);
const callWithRequest = callWithRequestFactory(request, server);
const indexExists = await getIndexExists(callWithRequest, finalIndex);
if (!indexExists) {
return createBulkErrorObject({
ruleId: ruleIdOrUuid,
statusCode: 400,
message: `To create a rule, the index must exist first. Index ${finalIndex} does not exist`,
});
}
if (ruleId != null) {
const rule = await readRules({ alertsClient, ruleId });
if (rule != null) {
return createBulkErrorObject({
ruleId,
statusCode: 409,
message: `rule_id: "${ruleId}" already exists`,
});
}
}
const createdRule = await createRules({
alertsClient,
actionsClient,
description,
enabled,
falsePositives,
from,
immutable: false,
query,
language,
outputIndex: finalIndex,
savedId,
timelineId,
timelineTitle,
meta,
filters,
ruleId: ruleIdOrUuid,
index,
interval,
maxSignals,
riskScore,
name,
severity,
tags,
to,
type,
threat,
references,
version,
});
return transformOrBulkError(ruleIdOrUuid, createdRule);
} catch (err) {
return transformBulkError(ruleIdOrUuid, err);
}
})
);
return rules;
if (dupes) {
return [
...rules,
...dupes.map(ruleId =>
createBulkErrorObject({
ruleId,
statusCode: 409,
message: `rule_id: "${ruleId}" already exists`,
})
),
];
}
return [...rules];
},
};
};
Expand Down
4 changes: 2 additions & 2 deletions x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1205,13 +1205,13 @@ describe('utils', () => {
});

describe('getDuplicates', () => {
test("returns a string showing the duplicate keys of 'value2' and 'value3'", () => {
test("returns array of ruleIds showing the duplicate keys of 'value2' and 'value3'", () => {
const output = getDuplicates({
value1: 1,
value2: 2,
value3: 2,
});
const expected = 'value2, value3';
const expected = ['value2', 'value3'];
expect(output).toEqual(expected);
});
test('returns null when given a map of no duplicates', () => {
Expand Down
6 changes: 2 additions & 4 deletions x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,12 +217,10 @@ export const transformOrImportError = (
}
};

export const getDuplicates = (lodashDict: Dictionary<number>): string | null => {
export const getDuplicates = (lodashDict: Dictionary<number>): string[] | null => {
const hasDuplicates = Object.values(lodashDict).some(i => i > 1);
if (hasDuplicates) {
return Object.keys(lodashDict)
.filter(key => lodashDict[key] > 1)
.join(', ');
return Object.keys(lodashDict).filter(key => lodashDict[key] > 1);
}
return null;
};
10 changes: 6 additions & 4 deletions x-pack/test/detection_engine_api_integration/security_and_spaces/tests/create_rules_bulk.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export default ({ getService }: FtrProviderContext): void => {
});

// TODO: This is a valid issue and will be fixed in an upcoming PR and then enabled once that PR is merged
it.skip('should return a 200 ok but have a 409 conflict if we attempt to create the same rule_id twice', async () => {
it('should return a 200 ok but have a 409 conflict if we attempt to create the same rule_id twice', async () => {
const { body } = await supertest
.post(`${DETECTION_ENGINE_RULES_URL}/_bulk_create`)
.set('kbn-xsrf', 'true')
Expand All @@ -89,9 +89,11 @@ export default ({ getService }: FtrProviderContext): void => {

expect(body).to.eql([
{
error: 'Conflict',
message: 'rule_id: "rule-1" already exists',
statusCode: 409,
error: {
message: 'rule_id: "rule-1" already exists',
status_code: 409,
},
rule_id: 'rule-1',
},
]);
});
Expand Down

0 comments on commit e2609c1

Please sign in to comment.