[Security Solution] Unable to reset to none if rule contains any timeline template under bulk apply timeline template

[ghost]

Describe the bug
Unable to reset to none if rule contains any timeline template under bulk apply timeline template

Build Details:

Version: 8.2.0 BC1
Commit:d18a093a2cf03991b93ea3de6a1054d580d3e82f
Build:51685

preconditions

1. Rule should be exist with any timeline template
2. Timeline templates should be exist

Steps to Reproduce

1. Navigate to Rules tab
2. Select any custom rule
3. Click bulk actions and then click on Apply timeline template
4. Select "None" from drop down
5. Navigate to rule details page
6. Observe that unable to reset to none if rule contains any timeline template under bulk apply timeline template

Actual Result
Unable to reset to none if rule contains any timeline template under bulk apply timeline template

Expected Result
User should be able to set None timeline template under bulk apply timeline template

Screen-Shot

Timeline.None.mp4

[elasticmachine]

Pinging @elastic/security-solution (Team: SecuritySolution)

[MadameSheema]

Closing since the functionality is not ready to be tested.

[banderror]

Yeah, it wasn't ready. Thank you @MadameSheema!
But I will reopen this one to reference it from #129491 🙂

[elasticmachine]

Pinging @elastic/security-detections-response (Team:Detections and Resp)

[banderror]

@deepikakeshav-qasource @MadameSheema so the backport to 8.2 has been merged, but I guess it won't be available in BC3. Could you please verify the fix in the 8.2 branch?

[ghost]

Hi @banderror ,

We have observed that this issue is occurring on Main branch. We are unable to reset to none if rule contains any timeline template under bulk apply timeline template.

Please find the below testing details:

Build Details

Branch: main
Commit: c572e3e2a3a19ca7a02aa2ae27fbb70678b3e8e6

Screencast:

bulk.timeline.mp4

Hence, We are re-opening this issue.

Thanks!!

[banderror]

Thank you @deepikakeshav-qasource. @vitaliidm you are in the context of bulk editing, could you please take a look at what we might break in main?

[vitaliidm]

Looks like ES _bulk update method(that used in rulesClient.bukEdit, which uses savedObjectClient.bulkUpdate under the hood) doesn't overwrite fields with undefined value.
https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-bulk.html

Here is actual query that is generated on bulkUpdate call

POST /_bulk?refresh=wait_for&_source_includes=originId&require_alias=true
{"update":{"_id":"alert:86d57b50-0ddb-11ed-a275-978547703fb5","_index":".kibana_8.4.0","if_seq_no":81720,"if_primary_term":1}}
{"doc":{"alert":{"name":"test 1","tags":[],"alertTypeId":"siem.queryRule","consumer":"siem","params":{"author":[],"description":"test 123","ruleId":"d0e41d77-ca63-4679-84a9-03a91f7410f4","falsePositives":[],"from":"now-360s","immutable":false,"license":"","outputIndex":"","meta":{"from":"1m","kibana_siem_app_url":"http://localhost:5601/kbn/app/security"},"maxSignals":100,"relatedIntegrations":[],"requiredFields":[],"riskScore":21,"riskScoreMapping":[],"setup":"","severity":"low","severityMapping":[],"threat":[],"to":"now","references":[],"version":22,"exceptionsList":[],"type":"query","language":"kuery","query":"rrr","filters":[],"index":["apm-*-transaction*","auditbeat-*","endgame-*","filebeat-*","logs-*","packetbeat-*","traces-apm*","winlogbeat-*","-*elastic-cloud-logs-*","test-index-*"]},"schedule":{"interval":"5m"},"enabled":true,"actions":[],"throttle":null,"notifyWhen":"onActiveAlert","apiKeyOwner":"elastic","legacyId":null,"createdBy":"elastic","updatedBy":"elastic","createdAt":"2022-07-27T18:40:03.020Z","updatedAt":"2022-07-28T10:58:31.327Z","snoozeSchedule":[],"muteAll":true,"mutedInstanceIds":[],"executionStatus":{"status":"error","lastExecutionDate":"2022-07-28T10:54:52.592Z","error":{"reason":"decrypt","message":"Unable to decrypt attribute \"apiKey\""},"warning":null,"lastDuration":18},"monitoring":{"execution":{"history":[{"duration":18,"success":false,"timestamp":1659005692611}],"calculated_metrics":{"success_ratio":0,"p99":18,"p50":18,"p95":18}}},"mapped_params":{"risk_score":21,"severity":"20-low"},"meta":{"versionApiKeyLastmodified":"8.4.0"},"scheduledTaskId":"86d57b50-0ddb-11ed-a275-978547703fb5","apiKey":"WJjva7JBTeqrGkW4kbCK86j52NqoN2doOD5drKTtfS2SWLxlccLErXMDRUHT7sX+zqPi6ZKKDHQz52Bnj7X8qCtBADF7mB9g8uOjwlk9VBFMr5Q54+pirPWdoizrYAXeiYEsUk2mGIeitrpmGS0EOX7P9+u2kJhs18wE6xxJDKPrVguz7oLIeAG390Kxi00jmzBo+9KCnwGvag=="},"updated_at":"2022-07-28T10:58:31.336Z","references":[]}}

This _bulk query can't set fileds to undefined.

Here is a simple example in Kibana dev tools to reproduce:

// create index
PUT /timeline-issue
{
  "settings": {
    "number_of_shards": 1
  },
  "mappings": {
    "properties": {
      "params": {  "type": "flattened" }
    }
  }
}

// add new SO
POST /timeline-issue/_doc/1
{
  "params": {
    "description": "test SO 1",
    "timelineTitle": "Test TT"
  }
}

// rewrite values in SO by using _bulk, see timelineTitle changed to new value
POST /_bulk?refresh=wait_for&_source=true
{"update":{"_id":"1","_index":"timeline-issue"}}
{"doc":{"params":{"description": "test SO 1","timelineTitle": "Test TT Bulk"}}}

// rewrite values in SO by using _bulk, see timelineTitle has not benn rewritten
POST /_bulk?refresh=wait_for&_source=true
{"update":{"_id":"1","_index":"timeline-issue"}}
{"doc":{"params":{"description": "test SO 2"}}, "doc_as_upsert" : true }

// simple put on contrary rewritse field if it's undefined
PUT /timeline-issue/_doc/1
{
  "params": {
    "description": "test SO 1"
  }
}

GET /timeline-issue/_doc/1

Possible solutions can be:

• to set value of timeline properties(timelineId and timelineTemplate) to null instead of undefined
  This, of course, would require changes across the app, where we will need to address type changing, and accommodate on UI displaying None, when rules don't have timeline properties.
• there is index action available, as per documentation: https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-bulk.html#bulk-api-request-body
  This might be the action that should be used in rulesClient to perform bulk update of saved object. But if it can be used there, it should be supported by savedObjectClient

cc: @XavierM , any ideas how we can address it in other ways?

[banderror]

@vitaliidm Thank you for doing this research and providing a detailed explanation. I tried bulk index in Dev Tools and it seems to work properly in this case. When called without timelineTitle, this field gets removed from the document:

POST /_bulk?refresh=wait_for&_source=true
{"index":{"_id":"1","_index":"timeline-issue"}}
{"params":{"description": "test SO 2"}}

{
  "_index": "timeline-issue",
  "_id": "1",
  "_version": 12,
  "_seq_no": 11,
  "_primary_term": 1,
  "found": true,
  "_source": {
    "params": {
      "description": "test SO 2"
    }
  }
}

If possible, it would be great if RulesClient could use index instead of update when bulk editing rules.

On the other hand, I think regardless of that, we should use explicitly defined values for timeline_id and timeline_title representing the None timeline template value.

Option 1:

timeline_id: null,
timeline_title: '',

Option 2:

timeline_id: '',
timeline_title: '',

Optional fields are great in some cases when you define request parameters (e.g. for the import endpoint), but showed to bring more problems than value when storing data.

[vitaliidm]

there is a possibility to use bulkCreate with overwrite=true
here is a PR with replacement #137593
let's see if we can get it in this release

@banderror why not 2 fields as null? It will give an immediate hint that timeline is set to None.

[banderror]

@vitaliidm both fields null is also an option, but timeline_title is just a text and can't be used for checking if the value is None or not (we should be checking timeline_id for that). If timeline_id === null, we reset timeline_title to an empty value. The safest empty value for a field that represents text (name, description, title, code block, etc) is an empty string. If you allow nullable values for a text field, you will need to make sure to check for them before reading the field; if you don't, you end up with null and undefined strings rendered in the UI and other similar bugs.

[vitaliidm]

I think having both nullable will be more consistent
In existing code for example
https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/public/detections/components/rules/description_step/index.tsx#L219
used Nullish coalescing operator. In my opinion it's very convenient way to check if value exists.

Having title set to '' would require additional checks if string is not empty. If title is null or undefined it means it does not exist at all and would not require any other verifications.

The safest empty value for a field that represents text (name, description, title, code block, etc) is an empty string

In case of timeline title, we don't display empty string, but predefined value None, which is removes any benefit of using empty string as value

[banderror]

@vitaliidm what if timeline_id === null and timeline_title === 'Generic Process Timeline'? Would you render it in the UI as Generic Process Timeline? If this is how it works in the code (and it looks like the current model allows that), I'd consider it a bug.

On the read side, e.g. when rendering the "Timeline" field on the Details page, we shouldn't be checking timeline_title for determining whether it's a None or a specified timeline template. We should be relying on timeline_id for that:

• if timeline_id === null we display None
• if timeline_id != null we display timeline_title; since timeline_title can't be null or undefined, we don't have to do any other checks like timeline_title ?? 'some default value'

[vitaliidm]

@vitaliidm what if timeline_id === null and timeline_title === 'Generic Process Timeline'? Would you render it in the UI as Generic Process Timeline? If this is how it works in the code (and it looks like the current model allows that), I'd consider it a bug.

@banderror , how is this even possible? By deliberate saving these values by direct API call? If we want to address this possibility: what should be displayed if timeline_id is invalid( timeline_id=== 'some-fake-id' timeline_title === 'Generic Process Timeline') or timeline_id and timeline_title are mismatched( timeline_id points to 'Generic Network Timeline' and timeline_title === 'Generic Process Timeline')?
To ensure it points to right timeline_title, we would need to fetch all timelines and find title in it. Only in this case we can be sure it covered. That raises question: do we need timeline_title then at all?

[vitaliidm]

@banderror, it has been fixed in #137593
I think we still want to make timeline properties nullable we have to open another ticket to address it and agreed upon the scope

@deepikakeshav-qasource , issue is ready for testing

[ghost]

Hi Team,

We have validated this issue on 8.4.0 BC2 build and observed that issue is Fixed. 🟢

Please find the below Testing Details:

Build info

VERSION : 8.4.0 BC2
Build: 55166
COMMIT: 9e9e0d6a685cbc2858a85a357f93dcb76259fdee

Screen-cast

timeline.template.mp4

cc: @MadameSheema

Hence, We are closing this issue and marking as QA Validated

Thanks!!