From 3158cd6c62aa3a9342a3635bd5e62f57e16976cf Mon Sep 17 00:00:00 2001 From: James Wu <88938117+thejwuscript@users.noreply.github.com> Date: Thu, 11 Apr 2024 02:48:38 +0900 Subject: [PATCH 1/3] Prevent same rule_id from concurrent import --- .../logic/import/import_rules_utils.test.ts | 72 +++++++++++++++++++ .../logic/import/import_rules_utils.ts | 33 +++++++-- 2 files changed, 101 insertions(+), 4 deletions(-) diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/import/import_rules_utils.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/import/import_rules_utils.test.ts index 7842febda6821..d8bda60780b14 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/import/import_rules_utils.test.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/import/import_rules_utils.test.ts @@ -18,10 +18,14 @@ import { import { createRules } from '../crud/create_rules'; import { updateRules } from '../crud/update_rules'; +import { deleteRules } from '../crud/delete_rules'; import { importRules } from './import_rules_utils'; +import type { SanitizedRule } from '@kbn/alerting-plugin/common'; + jest.mock('../crud/create_rules'); jest.mock('../crud/update_rules'); +jest.mock('../crud/delete_rules'); describe('importRules', () => { const mlAuthz = { @@ -39,6 +43,10 @@ describe('importRules', () => { jest.clearAllMocks(); }); + afterEach(() => { + (createRules as jest.Mock).mockReset(); + }); + it('returns rules response if no rules to import', async () => { const result = await importRules({ ruleChunks: [], @@ -74,6 +82,12 @@ describe('importRules', () => { }); it('creates rule if no matching existing rule found', async () => { + (createRules as jest.Mock).mockResolvedValue(getRuleMock(getQueryRuleParams())); + clients.rulesClient.find + .mockResolvedValue(getEmptyFindResult()) + .mockResolvedValueOnce(getEmptyFindResult()) + .mockResolvedValueOnce(getFindResultWithSingleHit()); + const result = await importRules({ ruleChunks: [[getImportRulesSchemaMock({ rule_id: 'rule-1' })]], rulesResponseAcc: [], @@ -250,4 +264,62 @@ describe('importRules', () => { }, ]); }); + + describe('when multiple rules with the same rule_id are found right after rule creation', () => { + const mockRuleId = '2a0e0ddd-8fba-49e2-b1bf-ff90be3e6fe1'; + const mockRuleOne = { + id: '97e61faf-dd3d-43bf-b1b5-0bf470423bb5', + } as SanitizedRule; + const mockRuleTwo = { + id: '3e592a38-5f88-43a9-b50e-b0236adecd67', + } as SanitizedRule; + + beforeEach(() => { + clients.rulesClient.find.mockResolvedValue( + getFindResultWithMultiHits({ data: [mockRuleOne, mockRuleTwo], total: 2 }) + ); + }); + + it('keeps the rule that was created first', async () => { + (createRules as jest.Mock).mockResolvedValue(mockRuleOne); + + const result = await importRules({ + ruleChunks: [[getImportRulesSchemaMock({ rule_id: mockRuleId })]], + rulesResponseAcc: [], + mlAuthz, + overwriteRules: false, + rulesClient: context.alerting.getRulesClient(), + existingLists: {}, + }); + + expect(result).toEqual([{ rule_id: mockRuleId, status_code: 200 }]); + expect(createRules).toHaveBeenCalled(); + expect(deleteRules).not.toHaveBeenCalled(); + }); + + it('deletes duplicate rules', async () => { + (createRules as jest.Mock).mockResolvedValue(mockRuleTwo); + + const result = await importRules({ + ruleChunks: [[getImportRulesSchemaMock({ rule_id: mockRuleId })]], + rulesResponseAcc: [], + mlAuthz, + overwriteRules: false, + rulesClient: context.alerting.getRulesClient(), + existingLists: {}, + }); + + expect(result).toEqual([ + { + rule_id: mockRuleId, + error: { + status_code: 409, + message: `rule_id: "${mockRuleId}" already exists`, + }, + }, + ]); + expect(createRules).toHaveBeenCalled(); + expect(deleteRules).toHaveBeenCalled(); + }); + }); }); diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/import/import_rules_utils.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/import/import_rules_utils.ts index 179c4069596e7..cda1318d2514c 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/import/import_rules_utils.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/import/import_rules_utils.ts @@ -20,6 +20,8 @@ import { createBulkErrorObject } from '../../../routes/utils'; import { createRules } from '../crud/create_rules'; import { readRules } from '../crud/read_rules'; import { updateRules } from '../crud/update_rules'; +import { deleteRules } from '../crud/delete_rules'; +import { findRules } from '../search/find_rules'; import type { MlAuthz } from '../../../../machine_learning/authz'; import { throwAuthzError } from '../../../../machine_learning/validation'; import { checkRuleExceptionReferences } from './check_rule_exception_references'; @@ -104,7 +106,7 @@ export const importRules = async ({ }); if (rule == null) { - await createRules({ + const createdRule = await createRules({ rulesClient, params: { ...parsedRule, @@ -112,10 +114,33 @@ export const importRules = async ({ }, allowMissingConnectorSecrets, }); - resolve({ - rule_id: parsedRule.rule_id, - status_code: 200, + // find rules with the same rule_id, in case of concurrent imports + const rules = await findRules({ + rulesClient, + filter: `alert.attributes.params.ruleId: "${parsedRule.rule_id}"`, + page: 1, + fields: undefined, + perPage: undefined, + sortField: 'created_at', + sortOrder: 'asc', }); + // OK if it is a unique rule or the earlier rule created + if (rules.total === 1 || createdRule.id === rules.data[0].id) { + resolve({ + rule_id: parsedRule.rule_id, + status_code: 200, + }); + // else delete the duplicate rule and return status 409 + } else { + deleteRules({ ruleId: createdRule.id, rulesClient }); + resolve( + createBulkErrorObject({ + ruleId: parsedRule.rule_id, + statusCode: 409, + message: `rule_id: "${parsedRule.rule_id}" already exists`, + }) + ); + } } else if (rule != null && overwriteRules) { await updateRules({ rulesClient, From 4dd06cc934a80c8d9b3d45fe22c66fa9071e7f83 Mon Sep 17 00:00:00 2001 From: James Wu <88938117+thejwuscript@users.noreply.github.com> Date: Fri, 12 Apr 2024 03:46:11 +0900 Subject: [PATCH 2/3] Fix failing tests --- .../api/rules/import_rules/route.test.ts | 57 ++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/api/rules/import_rules/route.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/api/rules/import_rules/route.test.ts index da4f9e67d7b8f..fbeb9aaa51be3 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/api/rules/import_rules/route.test.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/api/rules/import_rules/route.test.ts @@ -32,6 +32,9 @@ import * as createRulesAndExceptionsStreamFromNdJson from '../../../logic/import import { getQueryRuleParams } from '../../../../rule_schema/mocks'; import { importRulesRoute } from './route'; +import * as findRulesModule from '../../../logic/search/find_rules'; +import * as readRulesModule from '../../../logic/crud/read_rules'; + jest.mock('../../../../../machine_learning/authz'); describe('Import rules route', () => { @@ -138,6 +141,21 @@ describe('Import rules route', () => { }); describe('single rule import', () => { + let mockReadRules: jest.SpyInstance; + let mockFindRules: jest.SpyInstance; + + beforeAll(() => { + mockReadRules = jest.spyOn(readRulesModule, 'readRules').mockResolvedValue(null); + mockFindRules = jest + .spyOn(findRulesModule, 'findRules') + .mockResolvedValue(getFindResultWithSingleHit()); + }); + + afterAll(() => { + mockReadRules.mockRestore(); + mockFindRules.mockRestore(); + }); + test('returns 200 if rule imported successfully', async () => { clients.rulesClient.create.mockResolvedValue(getRuleMock(getQueryRuleParams())); const response = await server.inject(request, requestContextMock.convertContext(context)); @@ -187,6 +205,18 @@ describe('Import rules route', () => { }); describe('rule with existing rule_id', () => { + let mockReadRules: jest.SpyInstance; + + beforeEach(() => { + mockReadRules = jest + .spyOn(readRulesModule, 'readRules') + .mockResolvedValueOnce(getRuleMock(getQueryRuleParams())); + }); + + afterEach(() => { + mockReadRules.mockRestore(); + }); + test('returns with reported conflict if `overwrite` is set to `false`', async () => { clients.rulesClient.find.mockResolvedValue(getFindResultWithSingleHit()); // extant rule const response = await server.inject(request, requestContextMock.convertContext(context)); @@ -244,6 +274,23 @@ describe('Import rules route', () => { }); describe('multi rule import', () => { + let mockReadRules: jest.SpyInstance; + let mockFindRules: jest.SpyInstance; + + beforeAll(() => { + mockReadRules = jest.spyOn(readRulesModule, 'readRules').mockResolvedValue(null); + mockFindRules = jest + .spyOn(findRulesModule, 'findRules') + .mockResolvedValue(getFindResultWithSingleHit()); + clients.rulesClient.create.mockResolvedValue(getRuleMock(getQueryRuleParams())); + }); + + afterAll(() => { + clients.rulesClient.create.mockReset(); + mockReadRules.mockRestore(); + mockFindRules.mockRestore(); + }); + test('returns 200 if all rules imported successfully', async () => { const multiRequest = getImportRulesRequest( buildHapiStream(ruleIdsToNdJsonString(['rule-1', 'rule-2'])) @@ -398,8 +445,16 @@ describe('Import rules route', () => { }); describe('rules with existing rule_id', () => { + let mockReadRules: jest.SpyInstance; + beforeEach(() => { - clients.rulesClient.find.mockResolvedValueOnce(getFindResultWithSingleHit()); // extant rule + mockReadRules = jest + .spyOn(readRulesModule, 'readRules') + .mockResolvedValueOnce(getRuleMock(getQueryRuleParams())); + }); + + afterEach(() => { + mockReadRules.mockRestore(); }); test('returns with reported conflict if `overwrite` is set to `false`', async () => { From c12bfbd37097ffb907da441c001f5c442ee08bea Mon Sep 17 00:00:00 2001 From: James Wu <88938117+thejwuscript@users.noreply.github.com> Date: Sat, 13 Apr 2024 22:14:37 +0900 Subject: [PATCH 3/3] Delete redundant let declaration --- .../rule_management/api/rules/import_rules/route.test.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/api/rules/import_rules/route.test.ts b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/api/rules/import_rules/route.test.ts index fbeb9aaa51be3..eaadc1ccf8cdc 100644 --- a/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/api/rules/import_rules/route.test.ts +++ b/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/api/rules/import_rules/route.test.ts @@ -205,8 +205,6 @@ describe('Import rules route', () => { }); describe('rule with existing rule_id', () => { - let mockReadRules: jest.SpyInstance; - beforeEach(() => { mockReadRules = jest .spyOn(readRulesModule, 'readRules') @@ -445,8 +443,6 @@ describe('Import rules route', () => { }); describe('rules with existing rule_id', () => { - let mockReadRules: jest.SpyInstance; - beforeEach(() => { mockReadRules = jest .spyOn(readRulesModule, 'readRules')