Improve default configuration and reporting of Bandit email hijacking #20
Conversation
- add support for BANDIT_WHITELIST envvar setting to allow whitelisting of email recipient(s) using a comma-delimited list - support multiple TO addresses for hijacked emails by processing as a comma-delimited list - ALWAYS print out the BANDIT_EMAIL and BANDIT_WHITELIST settings since these are *very* important on sites where we are trying to apply email hijacking by using the `eamil_bandit` composable setting.
| @@ -1,5 +1,13 @@ | |||
| import os | |||
|
|
|||
| from django.conf import settings | |||
There was a problem hiding this comment.
@jmurty is this necessary, or wise, considering that this file is itself part of the settings?
If the ADMINS and SERVER_EMAIL settings are already defined in our base settings file, we can just access it immediately. But since we only need these settings to print them as help text, it might be better to put those print statements (and import django.conf.settings) in https://github.com/ixc/ixc-django-docker/blob/master/ixc_django_docker/bandit.py instead?
In that module, the settings will be finalised and any other subsequent adjustments to those settings (e.g. in develop.py or project settings which haven't been imported yet) will be available.
There was a problem hiding this comment.
Sounds reasonable, I'll move them @mrmachine
There was a problem hiding this comment.
Hmm, except it doesn't look like the ixc_django_docker.bandit module is loaded at application start-up. It may be lazy-loaded and not actually called unless/until a site tries to actually send an email, which is far too late to be applying the sanity-checks
There was a problem hiding this comment.
@mrmachine Since moving the print and exception lines to ixc_django_docker leaves them unrun until too late, I have pushed a change to load settings from locals() instead of doing the risky import of django.conf.settings
There was a problem hiding this comment.
@jmurty I think we don't need to use locals(). Those two variables are always set in ixc_django_docker/settings/base.py. I don't think we need to support attempts to use individual optional split settings modules without also using the base split settings modules.
It's probably ok 99% of the time where it is, but there is still a possibility for the actual value used by email-bandit to differ from the value we report in the settings module, if local settings or some other settings module alters those values afterwards. We could add ixc_django_docker as an INSTALLED_APP and create a AppConfig.setup() method to report any settings sanity check data at startup?
Or just leave it where it is as it is an edge case, but we can drop the locals().
There was a problem hiding this comment.
@mrmachine I have dropped the use of locals(). I'm not sure why you're opposed to them (?) but it should be fine either way.
I don't think it is worth adding a whole new ixc_django_docker app just to review, report on, and sanity-check settings. That's more complication than it's worth IMO
There was a problem hiding this comment.
@jmurty I am not against using locals(). You just mentioned in your commit message that this usage was a hack, and I was pointing out that either way it's unnecessary (as is the NameError check) because we can assume that the email_bandit.py split settings module will only ever be applied on top of the base.py split settings module, which will always defines both of these two variables (SERVER_EMAIL and ADMINS). The POST_OFFICE variable would still need to use locals() or NameError check, though.
Read setting variables directly from `locals()` instead, where they should be present. If not, the lookups fall back to default values. Using `locals()` is a hack, but less hacky than wrapping each setting lookup in a try/catch block to guard against them being unset in general, or unset by the time this composable setting file is loaded.
This PR applies the changes discussed and summarised in 552b37a#commitcomment-34364309