[Security Solution] Prevent same rule from being imported concurrently

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,16 @@ describe('Import rules route', () => {
     });
 
     describe('rule with existing rule_id', () => {
+      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 +272,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']))
@@ -399,7 +444,13 @@ describe('Import rules route', () => {
 
     describe('rules with existing rule_id', () => {
       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 () => {

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();
+    });
+  });
 });

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,18 +106,41 @@ export const importRules = async ({
               });
 
               if (rule == null) {
-                await createRules({
+                const createdRule = await createRules({
                   rulesClient,
                   params: {
                     ...parsedRule,
                     exceptions_list: [...exceptions],
                   },
                   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,