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

Set default value for email sender to empty value #5981

Merged
merged 1 commit into from Jul 18, 2019

Conversation

bjoernhaeuser
Copy link
Contributor

Description

The previous default (@example.org) is problematic for use cases where
the email server rejects sender domains which are not validated / owned
by the graylog server owners (e.g. amazon ses).

In our use case users often forgot to empty the value and left the
default in which resulted in notifications which where not sent out.

With this change the default is empty and if I understand the code
correctly the result would be that the default which is set in the
server configuration should be used.

How Has This Been Tested?

Fired up a test instance and took a look at the empty field value.

Screenshots (if appropriate):

Sorry, did not take any :(

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

The previous default (@example.org) is problematic for use cases where
the email server rejects sender domains which are not validated / owned
by the graylog server owners (e.g. amazon ses).

In our use case users often forgot to empty the value and left the
default in which resulted in notifications which where not sent out.

With this change the default is empty and if I understand the code
correctly the result would be that the default which is set in the
server configuration should be used.
@bjoernhaeuser
Copy link
Contributor Author

Rebased to current master.

@bjoernhaeuser
Copy link
Contributor Author

@florianvolle is there anything I can do to get this into the next release? :)

@edmundoa
Copy link
Contributor

edmundoa commented Jul 18, 2019

Hi @bjoernhaeuser,

Thank you for creating this PR and for your patience while we looked into this!

Here is a bit of information while I test your PR: We are working on a new alerting system, including new notifications, that will be released in Graylog 3.1. Since this change should also be done there, I created #6159 to be sure we don't forget. Feel free to add any missing information in case I missed something.

@edmundoa edmundoa self-assigned this Jul 18, 2019
@bjoernhaeuser
Copy link
Contributor Author

@edmundoa Thank you! #6159 looks good to me.

Copy link
Contributor

@edmundoa edmundoa left a comment

Choose a reason for hiding this comment

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

As you mentioned, an empty sender will default to the transport_email_from_email configuration option in the server config file. When that option is not set, an exception is thrown that is unfortunately not very clear on what went wrong.

Thinking about it, I think this is still better than the old behaviour, where the server set this example email address as sender and this all failed anyway.

@edmundoa edmundoa merged commit b6a1b99 into Graylog2:master Jul 18, 2019
@bjoernhaeuser bjoernhaeuser deleted the empty-default-sender branch July 18, 2019 11:55
@bjoernhaeuser
Copy link
Contributor Author

Thanks <3

@edmundoa
Copy link
Contributor

@bjoernhaeuser thank you for your contribution! 🎉

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.

None yet

3 participants