Request for Comment: settings growth configuring Email Backend.

[Carlton Gibson]

Hi all. 

Can I ask for guidance/comments please? 

Ticket 31885 Update SMTP Email Backend to use an SSLContext came in for which there's a PR adding `EMAIL_SSL_CAFILE` &co settings to match the existing EMAIL_USE_SSL &co settings. 

The PR looks fine in itself. 

I do wonder about the growth on the number of settings here. 

It looks to my eye to get out of hand.  

An alternate would be subclass EmailBackend, containing your customisations there. 

Alternatively we might group setting better than just the `GROUP_` prefix (namespaces being one honking great idea). 

Alternatively, "no it's fine, don't worry". 

I mentioned similar about the growth of security headers in a previous thread. 

Django uses settings. Great! No problem. If they didn't exist, you'd have to invent them. But to what extent do we say that new settings are the way forward vs having people compose their code with configured components? 

(If I add the words "Technical Board" does that get extra eyes on this? 🙂)

Thanks for the input! 

Kind Regards,

Carlton

[Michiel Beijen]

Hi Carlton,

On Tue, Nov 24, 2020 at 11:35 AM Carlton Gibson
<carlton...@gmail.com> wrote:

> Ticket 31885 Update SMTP Email Backend to use an SSLContext came in for which there's a PR adding `EMAIL_SSL_CAFILE` &co settings to match the existing EMAIL_USE_SSL &co settings.
>
> The PR looks fine in itself.
>
> I do wonder about the growth on the number of settings here.
> It looks to my eye to get out of hand.

I think the main problem is that the EMAIL_SSL_* settings map directly
to smtplib parameters, and the old ones are deprecated in Python
already since 3.6, but not yet in Django:

https://docs.python.org/3.9/library/smtplib.html#smtplib.SMTP.starttls
"Deprecated since version 3.6: keyfile and certfile are deprecated in
favor of context. Please use ssl.SSLContext.load_cert_chain() instead,
or let ssl.create_default_context() select the system’s trusted CA
certificates for you."

so EMAIL_SSL_CERTFILE and EMAIL_SSL_KEYFILE in Django should be marked
as deprecated; people using it should be urged to use the new
settings. If I'm not mistaken the old settings also do not do hostname
verification, which is a security problem.

So indeed, with adding the new settings there are a bit too much
EMAIL_WHATEVER settings, the old ones should be marked deprecated and
probably should have been marked as such some Django releases ago.
--
Michiel

[Carlton Gibson]

Hi Michiel.

On 25 Nov 2020, at 17:53, Michiel Beijen <michiel...@gmail.com> wrote:

the old ones should be marked deprecated and
probably should have been marked as such some Django releases ago.

There you are, super. 👍

Thank you for your input! 

Kind Regards,

Carlton

[Tim Graham]

I don't think settings create any harm. They seem cleaner than suggesting subclassing the backend. If I'm understanding correctly, that would force everyone to write something like:

class MySMTPBackend(smtp.EmailBackend)
    def __init__(self, *args, **kwargs):
        super().__init__(*args, keyfile='<my keyfile>', **kwargs)

That sort of code probably isn't suitable for `settings.py` itself which means configuration data like `'<my keyfile>'` is scattered elsewhere, or else developers are forced to write boilerplate and create their own settings to refer to `'<my keyfile>'`.

See also past discussion "Move SMTP / email settings to a dictionary" (wontfix)
https://code.djangoproject.com/ticket/22734

[Carlton Gibson]

Hey Tim, 

Thanks for following up here. 

Thanks also for the link to the previous discussion, very interesting. 

So, looking at that, this was first discussed 7 years ago, when there were (just) 

6 email related settings. 

I liked Jannis' initial reaction at the time: "Oh god, YES!!"

It didn't get accepted in the end for arguments along the lines of 

"Just for purity" and "No cost to adding an additional setting". 

All this time later though, I would argue that those judgements, whilst 

reasonable at the time, underplayed the costs. 

We currently have 11 `EMAIL_` settings in `global_settings.py`[0]. 

The suggested PR here[1] would add three more. And there's another PR[1] open 

suggesting adding yet another.  

That would make 15 `EMAIL_` prefixed settings. 

I would suggest that's too much. That it's time we changed tack. 

The issue for me is that we're failing to insulate our users from a whole 

swathe of unnecessary complexity that they shouldn't have to deal with. 

As it is every reader of `settings.txt` has to go through each of these

`EMAIL_` settings, parse them, and answer the questions _Does this apply to me?

And how?_. Instead of encapsulating the details of the SMTP, and now SSLContext

modules, we've added a set of pass-through parameters which force users to

address them. 

Particularly for the proposed SSLContext parameters, most user will never need

them — they'll just want the default `None` to use the default system CA certs.

So, we're adding to the cognitive load of all users, to address a use case

affecting only a small subgroup. 

I don't see/accept/agree with the "boilerplate" point. I need to create a subclass yes: 

```

from django.core.mail.backends.smtp import EmailBackend as SMTPBackend

class EmailBackend(SMTPBackend):

    def __init__(self, *args, **kwargs):

        # Set Environment kwargs here.

```

So it's an import and two lines for the class and init definitions. 

The trade-off for that is insulating the plurality of users for whom these 

advanced configuration options just simply aren't relevant. 

The pay-off is that, by moving some of the more esoteric options out of 

settings, we make Django's settings easier to navigate and comprehend for every 

body.

It's like a form or the Sorites Paradox[3]: it turns out that in project with the

lifespan of Django, the seemingly harmless, "There's little cost to adding one

more setting" fails the induction rule. 

FWIW: I think this same issue applies to the HTTP security headers thread,

which we're still thinking about. We should encapsulate these with sane

defaults, in a module that users can go and investigate, customise and apply,

with a single setting, rather than added an every longer list, which forces

every user to face decisions that we should have made for them, for ourselves.

So that's my concern. 

To address yours, I certainly don't think configuration details should be 

"scattered". 

I'd recommend a `project.conf.email` module (or package if needs be) containing

all the email related details. Keeping it all together makes it easier to

maintain. (It's locality of behaviour: Where's email stuff? It's all in there.)

I'd then have a `EmailBackend` subclass per environment and set it with the 

`EMAIL_BACKEND` setting. (Folks can use environment variables and all the rest 

of it as they see fit here, much as they do now.)

I think most users are using SMTP, and they **all** need to set

host/username/password, so those settings should likely remain.

The email section in `settings.txt` then says (roughly):

* Use `EMAIL_BACKEND`. 

* The following few convenience settings for `SMTP` are available. 

* See email docs for more advanced use-cases.

I think that would be a simplification for everyone, including those with the

more advanced use-cases. (If you were subclassing EmailBackend, I would expect

you to put all conf details in the subclass, rather than using the convenience

settings for some…)

Finally, just on the format here — I suggest a subclass because I think it's

simple. Discussion on the old ticket you linked and on the thread about HTTP

headers goes back and forth a bit about dicts, and tuples, and classes with

clever fallback behaviour, and so on. (It then get's stuck on those points.) 

I don't think we need to be committed on those points. A subclass is easy. It's

a couple of lines of "boilerplate" but unlike strings you get editor

autocomplete and validation. If someone comes up with a clever subclass that

handles a dict, or env vars or…, then folks can adopt that. 

I don't think the general principle, that we should insulate users (ourselves)

from having to deal with the advanced use-cases every time should depend on

needing to find a perfect serialisation format.

I hope all that makes sense. I appreciate that the simplest thing to do at any

given moment is to continue just adding settings. Looking another 7 years out,

I can't help but think that doing so fails on appropriate complexity

management. 

As I said on the PR, I was at the point of saying, yes OK, let's just add the

settings. Then reviewing the PR again I had the same conclusion, conferred with

Mariusz, commentted as I did, and now we're here.

It's a style thing right? We can live with it either way, but what should we do 

for the best? 

Thanks! 

Kind Regards,

Carlton

[0]: https://github.com/django/django/blob/548dce50cf548e777a0c34b5485a146a0606ae73/django/conf/global_settings.py#L189-L211

[1]: https://github.com/django/django/pull/13305/

[2]: https://github.com/django/django/pull/13728/

[3]: https://plato.stanford.edu/entries/sorites-paradox/

[Tim Graham]

I don't think there's a big cost to additional entries in docs/ref/setttings.txt. I think it's far more common to browse that document for needed functionality rather than to read the entire thing. There are 23 DATABASES 'TEST' settings that probably aren't used very frequently, yet I don't think it would be an improvement to remove all those and instead instruct users to subclass the database backend instead. In any case, the documentation has to be somewhere. If it's not a setting, then readers have to discover that functionality elsewhere. If we introduce some distinction between "commonly used options" and "less commonly used options", that creates extra work to figure out whether the option is considered "common" (setting) or "uncommon" (subclass). And further, some users may look no further than the settings page and never even discover the uncommon options.

If some functionality is so obscure that it doesn't merit a setting, perhaps it doesn't need to be in Django. If it's merely the number of settings that irk you, perhaps something like EMAIL_SSL_CONTEXT = {'cafile': ..., 'capath': ..., 'cadata': ...} would ameliorate your concerns in this case.

[Carlton Gibson]

Hey Tim. 

I'm not sure if there's anything I might say to persuade you. 🤔

> I don't think there's a big cost to additional...

Yes, and that's the same argument from 7 years ago, and at other times in between. 

I grant that at each point an additional setting (or three) is the least disruptive, but if one keeps doing that look where it ends. 

I submit that if we were somehow able to inject that there'd be 12-15 EMAIL_ settings into that conversation, back when there were 6, judgements would likely have been different. 

(Maybe not all of them but…)

> If it's merely the number of settings ...

It's not the number of settings per se, it's that we're failing to encapsulate unnecessary complexity for our users. We're not doing what we can to keep Django's API clean and easily comprehensible. 

In any given case it's clearly not fatal — you can always just add a setting, or a kwarg or a ... — but overtime you end up looking at the interface and thinking Why is it so complex? Why is every option of the underlying implementation exposed on the surface?  

> ...that creates extra work to figure out ... 

Yes, it does. But my point is that's not extra work. That's (just) precisely the (exact) work of keeping it maintainable/usable/comprehensible over the long-run. 

In particular case of EMAIL_ I don't think it's difficult. Most users will use the SMTP backend at some point or other. All of those need to set HOST &co. These days, we should default to USE_TLS (so move that into the backend), we can likely settle on a sensible default timeout to save every user a few cycles there, and the rest of them are advanced options (again for the backend).

I do think moving such would be an improvement, both to the code and the docs. It would make them both more coherent. Any user needing more than the basics will find what they need in the email docs, rather than needing to have both the email docs and the settings docs open. 

I'm reluctant to go down the path of discussing the format of proposed setting (be it dict or different setting or...) — these seem always to head into the long-grass, as there are different options, none of which resolve every concern — so we just end up adding the setting. The default SSLContext just is the right option for most users. Adding settings here is adding complexity for everybody that will only apply to a few. We should encapsulate that complexity, not ask folks to deal with it. If we can agree that, then we can settle the best overall approach to achieve it. Maybe there is a better option that just subclassing, but first, can we insulate users from needing to consider every configuration option, whether relevant to them or not? 

We already tread this line. (Ultimately, we do say folks should subclass the DB backend if they need to.) I'm just suggesting that we maintain it here. 

And I'd argue similar for the HTTP headers. We should encapsulate those details in a module that users don't need to look into the details of — at all — until they're ready for them.

I'll stop there. I'm not sure you feel the force of this kind of API Surface Area argument? 

As I said before, I was at the point of just saying OK let's have the new settings, hoping to deal with this later, but exposing the SSLContext config parameter struck/strikes me as such an egregious example of where we'd be doing it wrong that, as I said, I baulked — we make the API incrementally more complex for arguments which will rarely be used. 

Each individual change doesn't have a great cost — that's the problem — but they add up, in a way that does. 

(GOTO 10 😀) 

I'm imagining we still don't agree…

Kind Regards,

Carlton