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

Feature/mail incident status #921

Merged

Conversation

mkcorpc
Copy link
Contributor

@mkcorpc mkcorpc commented Dec 25, 2020

Please find enclosed the implemented responder.

[FR] Responder which sends a mail with a detailed incident status #920

"type": "string",
"multi": true,
"required": false,
"defaultValue": "Incident Case Notification: "
Copy link

Choose a reason for hiding this comment

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

this should be ["Incident Case Notification: "] or better ["[email protected]"]

Copy link
Contributor Author

@mkcorpc mkcorpc Jan 7, 2021

Choose a reason for hiding this comment

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

No fully correct. It should be a comma separated string, e.g. "[email protected]" or "[email protected],[email protected]".

Sorry it is my first pull request. Do I need to resolve the conversation right now?

Copy link

Choose a reason for hiding this comment

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

I don't have reviewer status so it's just a comment

I tried your plugin on thehive4 and it displays a list of separate character instead of a string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried your plugin on thehive4 and it displays a list of separate character instead of a string

I dont know what your a meaning with 'separate characters'. Do you speak about an configuration option?

The following mail is sent by the plugin:
mail

Copy link

Choose a reason for hiding this comment

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

image

it looks like this from the settings page

Copy link
Contributor

@dadokkio dadokkio Jan 8, 2021

Choose a reason for hiding this comment

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

I can confirm ntcong error and suggestion.
First line is with his fix "defaultValue": ["Incident Case Notification:"]
second whith standard code "defaultValue": "Incident Case Notification: "
image

but probably is easier to just remove defaultValue since is not so useful.

@dadokkio dadokkio changed the base branch from master to develop January 8, 2021 09:23
@dadokkio
Copy link
Contributor

dadokkio commented Jan 8, 2021

I'm going to test the responder now.
Quick suggestion, can you remove ssl, email and json from requirements? Those are standards libs and trying to install requirements with pip it tries to install wrong ssl lib instead.

@dadokkio
Copy link
Contributor

dadokkio commented Jan 8, 2021

Some issues during tests:

  • My case don't have data.updatedAt value set so it fails when tries to divide it by 1e3
  • I've seen that you have commented out server.send_message(msg) .. it should be ok since you put From and To in the msg so are not required as parameters. Also [mail_address] returns an error since a list is not a valid option
  • I've custom fields that are not set in this case, but the create_HTMLTable function returns error because i[1] is then None and cannot be concatenate
  • When you check for email in tags can you please check for both "mail=" and "mail:"? TH3 and TH4 have different behavior on tags management and you need to check both.

@mkcorpc
Copy link
Contributor Author

mkcorpc commented Jan 9, 2021

I have implemented the following changes:

  • Removed ssl, email and json from requirements
  • Added check if 'data.updatedAt' exists
  • Cleanup of the code at the end
  • Changed '[mail_address]' to 'mail_address'
  • Removed the comment 'server.send_message(msg)'
  • Added check if custom fields of type date are set
  • Checks now for tags prefixed with 'mail=' and 'mail:'
  • tested if responder is still working

@mkcorpc
Copy link
Contributor Author

mkcorpc commented Jan 9, 2021

Addjusted json configuration.

  • Deleted default value for tlp_amber_mail_addresses
  • Deleted default value for tlp_green_mail_domains
  • Responders reloaded and tested

@dadokkio
Copy link
Contributor

I was able to run the plugin 😄, I just put small comments in the code.
The multilined ().format syntax for text I proposed for the summary could be also used in email creation.

@mkcorpc
Copy link
Contributor Author

mkcorpc commented Jan 13, 2021

Sorry, but I don't know where i can find your comments. How can I see these?

@dadokkio
Copy link
Contributor

I updated your code with my and @ntcong suggestions. Let me know your opinion 👍
It seems all changed cause I've black formatting on save 😄

@mkcorpc
Copy link
Contributor Author

mkcorpc commented Jan 18, 2021

I have "accepted" all conversations.

@dadokkio dadokkio added this to the 3.0.0 milestone Jan 20, 2021
@dadokkio dadokkio merged commit 592e933 into TheHive-Project:develop Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants