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.

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

TheHive Connector for Cases #180931

Merged
merged 16 commits into from
Sep 10, 2024
Merged

Conversation

brijesh-elastic
Copy link
Contributor

@brijesh-elastic brijesh-elastic commented Apr 16, 2024

Summary

This PR implements the case support for TheHive connector.
Depends on : #180138

Screenshots

List of connectors
image

Configure thehive connector from cases
image

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Release notes

Support TheHive connector to cases

@brijesh-elastic brijesh-elastic requested a review from a team as a code owner April 16, 2024 14:14
@pmuellr
Copy link
Member

pmuellr commented Jun 13, 2024

Curious what the state of this is, as it's still asking for a review from ResponseOps. I don't think we've gotten any specific asks to review this, so not sure if this is kinda a POC, or if we really want to merge it. But it shows up on RO PR's to review, so if it's not currently active, would be nice to close it for now ...

@semd semd self-requested a review July 23, 2024 13:14
@semd semd added release_note:feature Makes this part of the condensed release notes v8.16.0 Feature:Cases Cases feature Feature:Actions/ConnectorTypes Issues related to specific Connector Types on the Actions Framework labels Jul 23, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops-cases (Feature:Cases)

@semd semd added the Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) label Jul 23, 2024
@semd
Copy link
Contributor

semd commented Jul 23, 2024

@elasticmachine merge upstream

@semd
Copy link
Contributor

semd commented Jul 23, 2024

Hey @brijesh-elastic I have started testing this but the code does not seem ready for review yet. I am seeing errors in the parameter form, and the connector does not appear in the Cases page, because it's not configured as such.

To test it I created a temporary branch from this one and merged the main implementation branch https://github.com/brijesh-elastic/kibana/tree/thehive_case_connector. Did I miss anything? Or should I wait until the first PR is merged before starting with this one?

@brijesh-elastic
Copy link
Contributor Author

To test it I created a temporary branch from this one and merged the main implementation branch https://github.com/brijesh-elastic/kibana/tree/thehive_case_connector. Did I miss anything? Or should I wait until the first PR is merged before starting with this one?

You can test on the temporary branch. It's good to go.

@semd
Copy link
Contributor

semd commented Aug 26, 2024

@elasticmachine merge upstream

Copy link
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

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

Hi @brijesh-elastic, I see the connector supportedFeatureIds is missing CasesConnectorFeatureId here

supportedFeatureIds: [
AlertingConnectorFeatureId,
SecurityConnectorFeatureId,
UptimeConnectorFeatureId,
],

Is it intentional? When I add the cases feature everything works well

Comment on lines 21 to 36
const mapTLP = (tlp: number | string): string => {
switch (tlp) {
case "0":
return 'CLEAR';
case "1":
return 'GREEN';
case "2":
return 'AMBER';
case "3":
return 'AMBER+STRICT';
case "4":
return 'RED';
default:
return 'AMBER';
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this function be extracted outside of the component implementation? There's no need to be re-defined on each render. Also, there are 2 variables named tlp in the same block, and the linter is complaining about it.

} = (theCase.connector.fields as ConnectorTheHiveTypeFields['fields']) ?? {};
return {
tags: theCase.tags,
tlp: typeof tlp === 'string' ? parseInt(tlp) : tlp,
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we have a rule for the radix parameter of the parseInt call, the linter is complaining about it. To fix it:

Suggested change
tlp: typeof tlp === 'string' ? parseInt(tlp) : tlp,
tlp: typeof tlp === 'string' ? parseInt(tlp, 10) : tlp,

@semd
Copy link
Contributor

semd commented Aug 26, 2024

@brijesh-elastic I've tested the connector, and everything is working fine. The only thing I miss is the case's status property is not synced. I noticed it is not in the mappings, but it feels a bit odd.

Marked as In progress in Kibana:
in progress in kibana

Not marked as In progress in TheHive:
in progress in thehive

Is there any reason not to sync this field?

@brijesh-elastic
Copy link
Contributor Author

Hi @brijesh-elastic, I see the connector supportedFeatureIds is missing CasesConnectorFeatureId here

supportedFeatureIds: [
AlertingConnectorFeatureId,
SecurityConnectorFeatureId,
UptimeConnectorFeatureId,
],

Is it intentional? When I add the cases feature everything works well

No, Will add in next commit.

@brijesh-elastic
Copy link
Contributor Author

brijesh-elastic commented Aug 27, 2024

@brijesh-elastic I've tested the connector, and everything is working fine. The only thing I miss is the case's status property is not synced. I noticed it is not in the mappings, but it feels a bit odd.

Marked as In progress in Kibana: in progress in kibana

Not marked as In progress in TheHive: in progress in thehive

Is there any reason not to sync this field?

Yes. In TheHive, the status field can have values such as New, InProgress, Duplicated, False Positive, InDeterminate, Other, and True Positive, which are different to the status values in Kibana. Therefore, we have decided not to sync the status field. Reference.

Initially, we faced an issue where the Update thehive incident option in the cases UI was being disable after the severity updation. So, Christos suggested the change(here) to resolve it. As a result of this change, status updates in the Kibana UI are now also triggering the Update thehive incident to being enable.

@semd
Copy link
Contributor

semd commented Aug 28, 2024

@elasticmachine merge upstream

Copy link
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

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

Changes have been desk tested and everything is working as expected. All comments and doubts were solved. Approving on behalf of Security Solution.
LGTM

@cnasikas cnasikas self-requested a review September 3, 2024 08:56
@cnasikas
Copy link
Member

cnasikas commented Sep 6, 2024

@elasticmachine merge upstream

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Tested and LGTM! Great work! I left some minor comments. Also, I noticed that the TLP text for the dropdown selection (x-pack/plugins/cases/public/components/connectors/thehive/case_fields.tsx) and the preview (x-pack/plugins/cases/public/components/connectors/thehive/case_fields_preview.tsx) are the same. Is it possible to have a common data structure that it is shared between the components in case we want to update the text in the future? Wdyt?

*/

export const TheHiveFieldsRt = rt.strict({
tlp: rt.union([rt.number, rt.string, rt.null]),
Copy link
Member

Choose a reason for hiding this comment

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

The schema of the tlp of the connector is number and not a string. I think we should keep the same convention here for cases (number or null). This way we can avoid doing parseInt(tlp, 10) on the backend. We may need to do it on the frontend as the select dropdown accepts only strings as values (as you do here x-pack/plugins/stack_connectors/public/connector_types/thehive/params_case.tsx).

'data-test-subj': 'tlp-field',
options: tlpOptions,
fullWidth: true,
hasNoInitialSelection: true,
Copy link
Member

Choose a reason for hiding this comment

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

Given that the field is required should we have a default value on the dropdown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, AMBER.

it('it formats correctly when fields do not exist ', async () => {
const invalidFields = { tags: ['tag1'], severity: 'low', connector: { fields: null } } as Case;
const res = await format(invalidFields, []);
expect(res).toEqual({ tlp: null, severity: 1, tags: ['tag1'] });
Copy link
Member

Choose a reason for hiding this comment

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

Should we default the tlp to something if it is a required field on TheHive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've set the default TLP to AMBER in the frontend, so it will always be present in the format function.

Copy link
Member

@cnasikas cnasikas Sep 9, 2024

Choose a reason for hiding this comment

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

Users can use the API and they can set the tlp to null. Does TheHive accept null as a TLP?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, TheHive will accept it as null and will default set it to AMBER.

Copy link
Member

Choose a reason for hiding this comment

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

Oh perfect then!

* 2.0.
*/

export enum TheHiveTLP {
Copy link
Member

@cnasikas cnasikas Sep 9, 2024

Choose a reason for hiding this comment

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

I think is better if we do

export enum TheHiveTLP {
  CLEAR = 0,
  GREEN = 1,
  AMBER = 2,
  "AMBER+STRICT" = 3, <--- I added the + symbol so the label can be used in the UI.
  RED = 4,
}

The reason is that by doing

Object.entries(TheHiveTLP).map(
  ([_, value], index) => ({
    text: value,
    value: index,
  })
);

we rely on the order of the enum (index) which may change or be different in the future. If we explicitly map the key to an integer we avoid this problem. The key can be used as the value to be shown in the UI and there is no need to rely on the order of the enum. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, That make more sense. Let me change it.

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for your patience with my review. I tested and is working as expected.

@cnasikas cnasikas enabled auto-merge (squash) September 10, 2024 09:03
@cnasikas cnasikas merged commit 32d7316 into elastic:main Sep 10, 2024
54 checks passed
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cases 807 812 +5

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
cases 487.2KB 491.1KB +3.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
cases 150.8KB 151.0KB +181.0B
Unknown metric groups

async chunk count

id before after diff
cases 30 32 +2

ESLint disabled line counts

id before after diff
cases 61 63 +2

Total ESLint disabled count

id before after diff
cases 78 80 +2

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @brijesh-elastic

@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 10, 2024
delanni added a commit that referenced this pull request Sep 11, 2024
## Summary
This test (#180931) was introduced
right before the version bump of user-events
(#189949).

This PR updates the one async call to be awaited, hopefully fixing the
test.

Solves: #192475
Unskipped tests are passing now:
https://buildkite.com/elastic/kibana-pull-request/builds/233177

---------

Co-authored-by: Antonio <antoniodcoelho@gmail.com>
gergoabraham pushed a commit to gergoabraham/kibana that referenced this pull request Sep 13, 2024
## Summary

This PR implements the case support for TheHive connector.
Depends on : elastic#180138

## Screenshots

**List of connectors**

![image](https://github.com/elastic/kibana/assets/123942796/7236eaed-d1d5-4506-80b1-c4e43c6ce36a)

**Configure thehive connector from cases**

![image](https://github.com/elastic/kibana/assets/123942796/bb4843d5-9f29-47c2-baa3-77db81a35319)


### Checklist

Delete any items that are not applicable to this PR.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Christos Nasikas <christos.nasikas@elastic.co>
gergoabraham pushed a commit to gergoabraham/kibana that referenced this pull request Sep 13, 2024
## Summary
This test (elastic#180931) was introduced
right before the version bump of user-events
(elastic#189949).

This PR updates the one async call to be awaited, hopefully fixing the
test.

Solves: elastic#192475
Unskipped tests are passing now:
https://buildkite.com/elastic/kibana-pull-request/builds/233177

---------

Co-authored-by: Antonio <antoniodcoelho@gmail.com>
markov00 pushed a commit to markov00/kibana that referenced this pull request Sep 18, 2024
## Summary
This test (elastic#180931) was introduced
right before the version bump of user-events
(elastic#189949).

This PR updates the one async call to be awaited, hopefully fixing the
test.

Solves: elastic#192475
Unskipped tests are passing now:
https://buildkite.com/elastic/kibana-pull-request/builds/233177

---------

Co-authored-by: Antonio <antoniodcoelho@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Actions/ConnectorTypes Issues related to specific Connector Types on the Actions Framework Feature:Cases Cases feature release_note:feature Makes this part of the condensed release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants