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

commented May 30, 2019

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.
Set default value for email sender to empty value
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 bjoernhaeuser force-pushed the bjoernhaeuser:empty-default-sender branch from 385b0cf to fe61a8e Jun 16, 2019

@bjoernhaeuser

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

Rebased to current master.

@bjoernhaeuser

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

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

@edmundoa

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

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

@edmundoa
Copy link
Member

left a comment

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

4 checks passed

ci-web-linter Jenkins build graylog-pr-linter-check 3727 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
graylog-project/pr Jenkins build graylog-project-pr-snapshot 4263 has succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@bjoernhaeuser bjoernhaeuser deleted the bjoernhaeuser:empty-default-sender branch Jul 18, 2019

@bjoernhaeuser

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

Thanks <3

@edmundoa

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

@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
3 participants
You can’t perform that action at this time.