Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution]Fix incorrect number of invalid connectors is shown on the toast message #152313

Merged
merged 10 commits into from
Mar 6, 2023
Merged

[Security Solution]Fix incorrect number of invalid connectors is shown on the toast message #152313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c75409b
initial
WafaaNasr Feb 23, 2023
a9e7772
Merge branch 'main' of https://github.com/elastic/kibana into fix-inc…
WafaaNasr Feb 27, 2023
4434471
fix incorrect number of connectors in the toast title
WafaaNasr Feb 28, 2023
7ade2cc
change variable name
WafaaNasr Feb 28, 2023
9f710a2
add des
WafaaNasr Feb 28, 2023
4d2e9a1
tests
WafaaNasr Feb 28, 2023
a2b46ef
explain the type casting in mapErrorMessage
WafaaNasr Mar 2, 2023
21308a4
Merge branch 'main' into fix-incorrect-invalid-connectors
kibanamachine Mar 2, 2023
bc5547c
add description for BulkError id and rule_id
WafaaNasr Mar 2, 2023
fa9a39e
Merge branch 'fix-incorrect-invalid-connectors' of https://github.com…
WafaaNasr Mar 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 95 additions & 0 deletions x-pack/plugins/security_solution/public/common/components/import_data_modal/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,102 @@ describe('showToasterMessage', () => {
expect(addSuccess).toHaveBeenNthCalledWith(2, 'Successfully imported 1 connector.');
expect(addError).not.toHaveBeenCalled();
});
it('should display 1 error message has 2 invalid connectors in the title even when error array has one message but "id" field', () => {
const addError = jest.fn();
const addSuccess = jest.fn();

showToasterMessage({
importResponse: {
success: false,
success_count: 1,
rules_count: 2,
action_connectors_success: false,
errors: [
{
rule_id: 'rule_id',
error: {
status_code: 400,
message: 'an error message',
},
},
],
action_connectors_errors: [
{
rule_id: 'rule_id',
id: 'connector1,connector2',
error: {
status_code: 400,
message: 'an error message',
},
},
],
exceptions_success: true,
exceptions_success_count: 0,
},
exceptionsIncluded: false,
actionConnectorsIncluded: true,
successMessage: (msg) => `success: ${msg}`,
errorMessage: (msg) => `error: ${msg}`,
errorMessageDetailed: (msg) => `errorDetailed: ${msg}`,
addError,
addSuccess,
});

expect(addError).toHaveBeenCalledTimes(1);

expect(addError).toHaveBeenCalledWith(new Error('errorDetailed: an error message'), {
title: 'Failed to import 2 connectors',
});
expect(addSuccess).not.toHaveBeenCalled();
});
it('should display 1 error message has 1 invalid connectors in the title even when error array has one message but "id" field', () => {
const addError = jest.fn();
const addSuccess = jest.fn();

showToasterMessage({
importResponse: {
success: false,
success_count: 1,
rules_count: 2,
action_connectors_success: false,
errors: [
{
rule_id: 'rule_id',
error: {
status_code: 400,
message: 'an error message',
},
},
],
action_connectors_errors: [
{
rule_id: 'rule_id',
id: 'connector1',
error: {
status_code: 400,
message: 'an error message',
},
},
],
exceptions_success: true,
exceptions_success_count: 0,
},
exceptionsIncluded: false,
actionConnectorsIncluded: true,
successMessage: (msg) => `success: ${msg}`,
errorMessage: (msg) => `error: ${msg}`,
errorMessageDetailed: (msg) => `errorDetailed: ${msg}`,
addError,
addSuccess,
});

expect(addError).toHaveBeenCalledTimes(1);

expect(addError).toHaveBeenCalledWith(new Error('errorDetailed: an error message'), {
title: 'Failed to import 1 connector',
});
expect(addSuccess).not.toHaveBeenCalled();
});
it('should display 1 error message for rules and connectors even when both fail', () => {
const addError = jest.fn();
const addSuccess = jest.fn();
Expand Down
19 changes: 14 additions & 5 deletions x-pack/plugins/security_solution/public/common/components/import_data_modal/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,14 @@ export const formatError = (
const mapErrorMessageToUserMessage = (
actionConnectorsErrors: Array<ImportRulesResponseError | ImportResponseError>
) => {
return actionConnectorsErrors.map((connectorError) => {
const { error } = connectorError;
let concatenatedActionIds: string = '';
const mappedErrors = actionConnectorsErrors.map((connectorError) => {
// Using "as ImportResponseError" because the "id" field belongs only to
// "ImportResponseError" and if the connectorError has the id we use it to get the
// number of failing connectors by spliting the unique the connectors ids.
const { id, error } = connectorError as ImportResponseError;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't it possible to just calculate the number of errors? Can we get more than one error per connector?

Generally speaking ImportRulesResponseError | ImportResponseError only differs by id and rule_id which come from BulkError (just the fact BulkError allows to omit both id and rule_id which is not supposed to happen I expect but it's another story). If we still support rule_id (most probably for backwards compatibility) can we have a situation when it's in use and this code is gonna fail? If it's not a case so then we should remove rule_id so as ImportResponseError type casting won't be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't it possible to just calculate the number of errors? Can we get more than one error per connector?

-Yes true.
Forex. if the user has rule to import in one file, and this rule has 3 connectors.

For instance, if 2 of them have missing connectors, what will happen as per the Old workflow=> One error message will be generated with the count of the failing connectors, and one error object will be pushed in the array.
2 connectors are missing. Connector ids missing are: X, Y

And just to clarify the id here is the action-connectors id not the rule_id

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @maximpn ! rule_id was initially created to act as a signature id so that a rule could be recognized, say, across platforms. This allows sigma, snort, suricata etc to identify a rule with a singular "signature id" that is never regenerated over time. So the concept of rule_id should hopefully stick around the solution unless platform has come up with a way for it to be supported more globally within a saved object. This comes in very handy on import when a user could be importing a rule they've ported over from elsewhere and want to maintain it's reference to the signature id. I don't think it would be in our interest to remove it's use. Like @WafaaNasr pointed out, the id she references is to the connector, the rule_id points back to the rule that had an action pointing to said connector so the user knows which rule was the source of the error.

If there's a worry that id would not exist in connectorError because it ends up being ImportRulesResponseError vs ImportResponseError maybe we can add a check for id != null.

If there's larger refactoring you think is necessary (this route is now a bit dated and could benefit from refactoring as it's complexity has grown a lot), feel free to create a ticket and we can keep discussing what the response should look like and how rule_id fits into the import use case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @yctercero, thank you for the explanation! My concern is misleading types as BulkError at backend and ImportRulesResponseError at frontend don't look related to the connectors. ImportResponseError looks too generic (by name). According to what I see in the code it's pretty easy to use some tailored for connectors error type which action_connectors_errors will use.

Absence of such a type leads to reusing id and rule_id fields to store serialized arrays in a case of missing connectors. I'm afraid it doesn't match with SO importer errors so that importer seems to returning an error per connector. Merging it now and creating a ticket looks like adding a technical debt while we already have it a lot.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's definitely refactoring needed overall on import. It used to just be rules and now is exceptions, actions and connectors. With the added comments, we can certainly look to simplify and refactor this flow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the concern with creating technical debt. As we are far into the BCs, adding scope in this bug fix can also open us up to further bugs. Our team is working on documenting this route, the technical debt and suggested updates. I can be sure to make reference back to your concerns to ensure they're addressed in any upcoming refactors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we go #152648

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @maximpn !

concatenatedActionIds =
concatenatedActionIds && concatenatedActionIds !== id ? `${concatenatedActionIds},${id}` : id;
const { status_code: statusCode, message: originalMessage } = error || {};
let message;
switch (statusCode) {
Expand All @@ -47,9 +53,12 @@ const mapErrorMessageToUserMessage = (

break;
}

return { ...connectorError, error: { ...error, message } };
});
const actionIds: Set<string> = new Set(
concatenatedActionIds && [...concatenatedActionIds.split(',')]
);
return { mappedErrors, numberOfActions: actionIds.size };
};

export const showToasterMessage = ({
Expand Down Expand Up @@ -100,13 +109,13 @@ export const showToasterMessage = ({
importResponse.action_connectors_errors != null &&
importResponse.action_connectors_errors.length > 0
) {
const userErrorMessages = mapErrorMessageToUserMessage(
const { mappedErrors: userErrorMessages, numberOfActions } = mapErrorMessageToUserMessage(
importResponse.action_connectors_errors
);
const connectorError = formatError(errorMessageDetailed, importResponse, userErrorMessages);

return addError(connectorError, {
title: i18n.IMPORT_CONNECTORS_FAILED(userErrorMessages.length),
title: i18n.IMPORT_CONNECTORS_FAILED(numberOfActions || userErrorMessages.length),
});
}
const ruleError = formatError(errorMessageDetailed, importResponse, importResponse.errors);
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/security_solution/server/lib/detection_engine/routes/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ export interface OutputError {
statusCode: number;
}
export interface BulkError {
// Id can be single id or stringified ids.
id?: string;
// rule_id can be single rule_id or stringified rules ids.
rule_id?: string;
error: {
status_code: number;
Expand Down
3 changes: 3 additions & 0 deletions ...gine/rule_management/logic/import/action_connectors/import_rule_action_connectors.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ describe('importRuleActionConnectors', () => {
'1 connector is missing. Connector id missing is: cabc78e0-9031-11ed-b076-53cc4d57aaf1',
status_code: 404,
},
id: 'cabc78e0-9031-11ed-b076-53cc4d57aaf1',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, you provide both id and rule_id in tests while it should be one of them according to the operated types. See my question regarding them above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are different id is the actionConnectors ids and the rule_id is the rule to be imported.

And we need it, in case the user has multiple rules in one file and not all of them are failing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. For some reason actionsImporter returns a comma separated list of connector identifiers in id field and we don't have any breakdown on this. So this list corresponds to a rule.

rule_id: 'rule-1',
},
],
Expand Down Expand Up @@ -220,6 +221,7 @@ describe('importRuleActionConnectors', () => {
status_code: 404,
},
rule_id: 'rule-1',
id: 'cabc78e0-9031-11ed-b076-53cc4d57aaf1,cabc78e0-9031-11ed-b076-53cc4d57aaf2',
},
],
warnings: [],
Expand Down Expand Up @@ -270,6 +272,7 @@ describe('importRuleActionConnectors', () => {
status_code: 404,
},
rule_id: 'rule-1,rule-2',
id: 'cabc78e0-9031-11ed-b076-53cc4d57aaf1,cabc78e0-9031-11ed-b076-53cc4d57aaf2',
},
],
warnings: [],
Expand Down
1 change: 1 addition & 0 deletions ...server/lib/detection_engine/rule_management/logic/import/action_connectors/utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export const handleActionsHaveNoConnectors = (
: 'connector is missing. Connector id missing is:';
errors.push(
createBulkErrorObject({
id: actionsIds.join(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As id field stores a serialized array it worth to have a comment in BulkError and ImportResponseError to highlight that fact it can contain more than one identifier in a case of missing connectors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll go ahead and add a comment there. Looks like rule_id has been used this way for a bit. Will be sure to document.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review bc5547c

statusCode: 404,
message: `${actionsIds.length} ${errorMessage} ${actionsIds.join(', ')}`,
ruleId: ruleIds,
Copy link
Contributor

@maximpn maximpn Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As rule_id field stores a serialized array it worth to have a comment in BulkError and ImportRuleResponseError to highlight that fact it can contain more than one identifier in a case of missing connectors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll go ahead and add a comment there. Looks like rule_id has been used this way for a bit. Will be sure to document.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please review here bc5547c

Expand Down
4 changes: 4 additions & 0 deletions x-pack/test/detection_engine_api_integration/security_and_spaces/group10/import_rules.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,6 +866,7 @@ export default ({ getService }: FtrProviderContext): void => {
errors: [
{
rule_id: 'rule-1',
id: '123',
error: {
status_code: 404,
message: '1 connector is missing. Connector id missing is: 123',
Expand All @@ -881,6 +882,7 @@ export default ({ getService }: FtrProviderContext): void => {
action_connectors_errors: [
{
rule_id: 'rule-1',
id: '123',
error: {
status_code: 404,
message: '1 connector is missing. Connector id missing is: 123',
Expand Down Expand Up @@ -1153,6 +1155,7 @@ export default ({ getService }: FtrProviderContext): void => {
errors: [
{
rule_id: 'rule-2',
id: 'cabc78e0-9031-11ed-b076-53cc4d57aa22',
error: {
status_code: 404,
message:
Expand All @@ -1173,6 +1176,7 @@ export default ({ getService }: FtrProviderContext): void => {
'1 connector is missing. Connector id missing is: cabc78e0-9031-11ed-b076-53cc4d57aa22',
},
rule_id: 'rule-2',
id: 'cabc78e0-9031-11ed-b076-53cc4d57aa22',
},
],
action_connectors_warnings: [],
Expand Down