Email TLS fixes - Githubissues

geopython / GeoHealthCheck

Service Status and QoS Checker for OGC Web Services

https://geohealthcheck.org

MIT License

84 stars 71 forks source link

Email TLS fixes #416

Closed tomkralidis closed 2 years ago

tomkralidis commented 2 years ago

This PR fixes a bug in GHC's Docker setup of GHC_SMTP_* environment variables that were not getting cast as booleans, and resulting in 'False' values evaluating to True in GHC notifications. In addition, TLS handling has been streamlined for better flow and error handling.

tomkralidis commented 2 years ago

CI errors seem unrelated.

tomkralidis commented 2 years ago

Thanks for the feedback @justb4. Updates rebased/pushed for review.

tomkralidis commented 2 years ago

@justb4 actually, testing this against an SMTP server that does not support AUTH still results in error. Options:

try to login if TLS is set in config
try to login evn if TLS is not set, on error throw warning instead of exception, and carry on and try to send mail
other options?

tomkralidis commented 2 years ago

Note: I've updated the PR now to implement option 2.

justb4 commented 2 years ago

login : in the current situation the exception is logged but ignored further. So I guess that is taken care. But I think a real fix is to optionally call login regardless of tls on the condition that a username/password is configured. Note that also the config check on lines 81-86 needs to be adapted.

To make things worse: I just saw that there is also a email function in util.py:L177 (where also tls and login is combined). That function is e.g. used for lost password.

The ultimate solution I see:

have a single send_email() function, e.g. in util.py
make it robust on all config conditions: auth/no-auth, tls etc like the aim of this PR

Later: use a higher-level lib for sending email. e.g. with FastAPI I use FastAPI-mail which you just provide a config and it takes care of all those details. SMTPLib is too low-level. On the other hand, we have one dependency less...

tomkralidis commented 2 years ago

@justb4 I've added testing for username and password settings. I also had to add a str2None function in docker/config_site.py to be able to cast environment variables that are 'None' as None.

At this point I think there is enough in this PR to cover the intent/fix. Agree that we should plan for streamlining email handling as part of a future PR.

justb4 commented 2 years ago

Strictly speaking, if username + password is set and login fails we should return (now only exception logging). Ok, but the sendmail will fail then anyway and this is the current behaviour.

Note that the util.py send_mail() function is also used to mail a daily summary...

But let's proceed/merge and open an issue to streamline all email functionality.

justb4 commented 2 years ago

I'll open the issue.