[Entity Analytics] Improve response body for asset criticality delete…

… route (elastic#189872) While doing the API docs I noticed the asset criticality delete route did not return a response body, and returned a very ugly 404 if you attempted to delete something which didnt exist... ``` {"message":"{\"_index\":\".asset-criticality.asset-criticality-default\",\"_id\":\"host.name:my_host\",\"_version\":1,\"result\":\"not_found\",\"_shards\":{\"total\":2,\"successful\":1,\"failed\":0},\"_seq_no\":0,\"_primary_term\":1}","full_error":"{\"name\":\"ResponseError\",\"message\":\"{\\\"_index\\\":\\\".asset-criticality.asset-criticality-default\\\",\\\"_id\\\":\\\"host.name:my_host\\\",\\\"_version\\\":1,\\\"result\\\":\\\"not_found\\\",\\\"_shards\\\":{\\\"total\\\":2,\\\"successful\\\":1,\\\"failed\\\":0},\\\"_seq_no\\\":0,\\\"_primary_term\\\":1}\"}","status_code":404}% ``` I have now made it so that we return a boolean for if the record was deleted or not (`false` if the record didnt exist) and we also return the record that was deleted if it was, e.g... ``` { "deleted": true, "record": { "id_field": "host.name", "id_value": "my_host", "criticality_level": "medium_impact", "@timestamp": "2024-08-05T09:42:11.240Z" } } ``` And if the record didnt exist... ``` { "deleted": false, } ``` --------- Co-authored-by: kibanamachine <[email protected]>
gsoldevila · Aug 5, 2024 · 70a4ad4 · 70a4ad4
1 parent 23a4e9b
commit 70a4ad4
Show file tree

Hide file tree

Showing 7 changed files with 109 additions and 13 deletions.
diff --git a/...ty_solution/common/api/entity_analytics/asset_criticality/delete_asset_criticality.gen.ts b/...ty_solution/common/api/entity_analytics/asset_criticality/delete_asset_criticality.gen.ts
@@ -16,7 +16,7 @@
 
 import { z } from 'zod';
 
-import { IdField } from './common.gen';
+import { IdField, AssetCriticalityRecord } from './common.gen';
 
 export type DeleteAssetCriticalityRecordRequestQuery = z.infer<
   typeof DeleteAssetCriticalityRecordRequestQuery
@@ -38,3 +38,14 @@ export const DeleteAssetCriticalityRecordRequestQuery = z.object({
 export type DeleteAssetCriticalityRecordRequestQueryInput = z.input<
   typeof DeleteAssetCriticalityRecordRequestQuery
 >;
+
+export type DeleteAssetCriticalityRecordResponse = z.infer<
+  typeof DeleteAssetCriticalityRecordResponse
+>;
+export const DeleteAssetCriticalityRecordResponse = z.object({
+  /**
+   * If the record was deleted. If false the record did not exist.
+   */
+  deleted: z.boolean(),
+  record: AssetCriticalityRecord.optional(),
+});
diff --git a/...lution/common/api/entity_analytics/asset_criticality/delete_asset_criticality.schema.yaml b/...lution/common/api/entity_analytics/asset_criticality/delete_asset_criticality.schema.yaml
@@ -33,5 +33,17 @@ paths:
       responses:
         '200':
           description: Successful response
+          content:
+            application/json:
+              schema:
+                type: object
+                properties:
+                  deleted:
+                    type: boolean
+                    description: If the record was deleted. If false the record did not exist.
+                  record:
+                    $ref: './common.schema.yaml#/components/schemas/AssetCriticalityRecord'
+                required:
+                  - deleted
         '400':
           description: Invalid request
diff --git a/...on/docs/openapi/ess/security_solution_entity_analytics_api_2023_10_31.bundled.schema.yaml b/...on/docs/openapi/ess/security_solution_entity_analytics_api_2023_10_31.bundled.schema.yaml
@@ -38,6 +38,20 @@ paths:
             type: string
       responses:
         '200':
+          content:
+            application/json:
+              schema:
+                type: object
+                properties:
+                  deleted:
+                    description: >-
+                      If the record was deleted. If false the record did not
+                      exist.
+                    type: boolean
+                  record:
+                    $ref: '#/components/schemas/AssetCriticalityRecord'
+                required:
+                  - deleted
           description: Successful response
         '400':
           description: Invalid request

diff --git a/.../openapi/serverless/security_solution_entity_analytics_api_2023_10_31.bundled.schema.yaml b/.../openapi/serverless/security_solution_entity_analytics_api_2023_10_31.bundled.schema.yaml
@@ -38,6 +38,20 @@ paths:
             type: string
       responses:
         '200':
+          content:
+            application/json:
+              schema:
+                type: object
+                properties:
+                  deleted:
+                    description: >-
+                      If the record was deleted. If false the record did not
+                      exist.
+                    type: boolean
+                  record:
+                    $ref: '#/components/schemas/AssetCriticalityRecord'
+                required:
+                  - deleted
           description: Successful response
         '400':
           description: Invalid request

diff --git a/...y_solution/server/lib/entity_analytics/asset_criticality/asset_criticality_data_client.ts b/...y_solution/server/lib/entity_analytics/asset_criticality/asset_criticality_data_client.ts
@@ -272,12 +272,37 @@ export class AssetCriticalityDataClient {
     return { errors, stats };
   };
 
-  public async delete(idParts: AssetCriticalityIdParts, refresh = 'wait_for' as const) {
-    await this.options.esClient.delete({
-      id: createId(idParts),
-      index: this.getIndex(),
-      refresh: refresh ?? false,
-    });
+  public async delete(
+    idParts: AssetCriticalityIdParts,
+    refresh = 'wait_for' as const
+  ): Promise<AssetCriticalityRecord | undefined> {
+    let record: AssetCriticalityRecord | undefined;
+    try {
+      record = await this.get(idParts);
+    } catch (err) {
+      if (err.statusCode === 404) {
+        return undefined;
+      } else {
+        throw err;
+      }
+    }
+
+    if (!record) {
+      return undefined;
+    }
+
+    try {
+      await this.options.esClient.delete({
+        id: createId(idParts),
+        index: this.getIndex(),
+        refresh: refresh ?? false,
+      });
+    } catch (err) {
+      this.options.logger.error(`Failed to delete asset criticality record: ${err.message}`);
+      throw err;
+    }
+
+    return record;
   }
 
   public formatSearchResponse(response: SearchResponse<AssetCriticalityRecord>): {

diff --git a/.../plugins/security_solution/server/lib/entity_analytics/asset_criticality/routes/delete.ts b/.../plugins/security_solution/server/lib/entity_analytics/asset_criticality/routes/delete.ts
@@ -4,10 +4,11 @@
  * 2.0; you may not use this file except in compliance with the Elastic License
  * 2.0.
  */
-import type { Logger } from '@kbn/core/server';
+import type { IKibanaResponse, Logger } from '@kbn/core/server';
 import { buildSiemResponse } from '@kbn/lists-plugin/server/routes/utils';
 import { transformError } from '@kbn/securitysolution-es-utils';
 import { buildRouteValidationWithZod } from '@kbn/zod-helpers';
+import type { DeleteAssetCriticalityRecordResponse } from '../../../../../common/api/entity_analytics';
 import { DeleteAssetCriticalityRecordRequestQuery } from '../../../../../common/api/entity_analytics';
 import {
   ASSET_CRITICALITY_PUBLIC_URL,
@@ -42,7 +43,11 @@ export const assetCriticalityPublicDeleteRoute = (
           },
         },
       },
-      async (context, request, response) => {
+      async (
+        context,
+        request,
+        response
+      ): Promise<IKibanaResponse<DeleteAssetCriticalityRecordResponse>> => {
         const securitySolution = await context.securitySolution;
 
         securitySolution.getAuditLogger()?.log({
@@ -61,15 +66,20 @@ export const assetCriticalityPublicDeleteRoute = (
           await checkAndInitAssetCriticalityResources(context, logger);
 
           const assetCriticalityClient = securitySolution.getAssetCriticalityDataClient();
-          await assetCriticalityClient.delete(
+          const deletedRecord = await assetCriticalityClient.delete(
             {
               idField: request.query.id_field,
               idValue: request.query.id_value,
             },
             request.query.refresh
           );
 
-          return response.ok();
+          return response.ok({
+            body: {
+              deleted: deletedRecord !== undefined,
+              record: deletedRecord,
+            },
+          });
         } catch (e) {
           const error = transformError(e);
 

diff --git a/...test_suites/entity_analytics/risk_engine/trial_license_complete_tier/asset_criticality.ts b/...test_suites/entity_analytics/risk_engine/trial_license_complete_tier/asset_criticality.ts
@@ -442,7 +442,7 @@ export default ({ getService }: FtrProviderContext) => {
     });
 
     describe('delete', () => {
-      it('should correctly delete asset criticality', async () => {
+      it('should correctly delete asset criticality if it exists', async () => {
         const assetCriticality = {
           id_field: 'host.name',
           id_value: 'delete-me',
@@ -451,7 +451,10 @@ export default ({ getService }: FtrProviderContext) => {
 
         await assetCriticalityRoutes.upsert(assetCriticality);
 
-        await assetCriticalityRoutes.delete('host.name', 'delete-me');
+        const res = await assetCriticalityRoutes.delete('host.name', 'delete-me');
+
+        expect(res.body.deleted).to.eql(true);
+        expect(_.omit(res.body.record, '@timestamp')).to.eql(assetCriticality);
         const doc = await getAssetCriticalityDoc({
           idField: 'host.name',
           idValue: 'delete-me',
@@ -461,6 +464,13 @@ export default ({ getService }: FtrProviderContext) => {
         expect(doc).to.eql(undefined);
       });
 
+      it('should not return 404 if the asset criticality does not exist', async () => {
+        const res = await assetCriticalityRoutes.delete('host.name', 'doesnt-exist');
+
+        expect(res.body.deleted).to.eql(false);
+        expect(res.body.record).to.eql(undefined);
+      });
+
       it('should return 403 if the advanced setting is disabled', async () => {
         await disableAssetCriticalityAdvancedSetting(kibanaServer, log);