Use Django's Email Functionality #220

thebeanogamer · 2021-10-30T18:03:35Z

Currently we have a completely custom mail backend which was ripped from ractf/mail-usv and as a result barely works. This PR replaces that with Django's internal mail backend combined with Anymail. This means that instead of just supporting SMTP and SES (and Sendgrid, but it was never tested), we now support all of the following:

SMTP
AWS
MAILGUN
SENDGRID
CONSOLE
MAILJET
MANDRILL
POSTAL
POSTMARK
SENDINBLUE
SPARKPOST
TEST
DISABLED

I've tested that SMTP and AWS work, as these are the ones we officially use and I'm reasonably confident that the others will work as that's abstracted away by Anymail. I've kept the existing environment variable names, so this should be backwards compatible.

Given that we can now inline images, I've also put the RACTF logo back into emails.

I've also added a dev mail page to /api/v2/admin/list/ which is only enabled when the anymail.backends.test.EmailBackend email backend is used, which will show the emails the platform has sent. This should make testing around emails easier.

Resolves #217

This doesn't work yet, as it depends on Django's builtin email stuff. That'll come next

codecov · 2021-10-30T18:05:56Z

Codecov Report

Merging #220 (791c9e9) into master (000fa7e) will increase coverage by 0.00%.
The diff coverage is 96.96%.

@@           Coverage Diff           @@
##           master     #220   +/-   ##
=======================================
  Coverage   94.38%   94.38%           
=======================================
  Files         107      107           
  Lines        4699     4721   +22     
  Branches      270      270           
=======================================
+ Hits         4435     4456   +21     
- Misses        221      222    +1     
  Partials       43       43

Impacted Files	Coverage Δ
src/authentication/tests.py	`100.00% <ø> (ø)`
src/admin/views.py	`93.33% <75.00%> (-6.67%)`	⬇️
src/authentication/serializers.py	`100.00% <100.00%> (ø)`
src/authentication/views.py	`100.00% <100.00%> (ø)`
src/backend/mail.py	`100.00% <100.00%> (ø)`
src/backend/settings/__init__.py	`76.54% <100.00%> (+3.68%)`	⬆️
src/backend/settings/lint.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 000fa7e...791c9e9. Read the comment docs.

Currently just ses, mailgun, and sendgrid. We can add more easily.

0xAda

Please don't make a new app for this, it will just lead to merge conflicts when we move it into core in app restructure. IMO the best place for now is in the admin app

thebeanogamer · 2021-10-30T20:35:27Z

Moving it into admin is easy enough, will do that

docker-compose.yml

Now we assume email is disabled if `EMAIL_PROVIDER` is unset, and send emails to `django.core.mail.backends.dummy.EmailBackend` instead.

thebeanogamer · 2021-10-31T00:16:17Z

I'm going to have another look at this once I'm awake and try and port the email templates to Django (which will also mean we can inline images), but this is more or less done.

404dcd · 2021-10-31T17:59:32Z

Have you considered #12

thebeanogamer · 2021-10-31T18:00:20Z

@404dcd This PR is just to resolve #217

thebeanogamer · 2021-10-31T18:13:53Z

I'm fairly certain codecov is wrong here.

How was this in the code base before...

thebeanogamer · 2021-12-11T16:17:06Z

What on earth is this

core/src/authentication/views.py

Lines 203 to 207 in 5ab5e9f

    
           except get_user_model().DoesNotExist: 
        
               password_reset_start_reject.send(sender=self.__class__, email=email) 
        
               uid = -1 
        
               token = "" 
        
               email = "noreply@ractf.co.uk"

Feels like we should alert the user somehow if they've not set this

src/authentication/serializers.py

0xAda

LGTM!

jchristgit · 2022-01-11T20:30:29Z

src/backend/settings/__init__.py

+        "BACKEND": "django.template.backends.jinja2.Jinja2",  # TODO: This is terrible
+        "DIRS": [os.path.join(BASE_DIR, "templates")],
+        "APP_DIRS": True,
+    },


It would be great if we could remove this in a subsequent PR.

jchristgit

Thanks!

jchristgit · 2022-01-11T20:31:00Z

Hi @thebeanogamer, can you check the code coverage error?

thebeanogamer · 2022-01-15T00:36:09Z

Remaining work here:

Fix coverage (or just bypass)
Update error handling

Work out what on earth this does

core/src/authentication/views.py

Lines 203 to 207 in 5ab5e9f

    
           except get_user_model().DoesNotExist: 
        
               password_reset_start_reject.send(sender=self.__class__, email=email) 
        
               uid = -1 
        
               token = "" 
        
               email = "noreply@ractf.co.uk"

Stale

jchristgit

You really knocked it out of the park this time. Just like you did last time,
and the time before that. Knowing you and that drive of yours, we’ll be
celebrating you again next week! Congratulations on this achievement. I already
know it will be one of many.

I am not sure how, but you did. Actually, now that I think about it, how did
you do it? Maybe one day you will share your secrets, and at least one person
will be able to compete with you (preferably me). Congratulations. You well
deserve it.

* Add Django Anymail * Don't just disable email, use Django's builtin * Add a page showing sent emails This doesn't work yet, as it depends on Django's builtin email stuff. That'll come next * Remove all our custom email code * Actually install the Mail app * Remove unused tests file * Remove now-needed code from staging.py * Remove dumb import catching * Remove unused imports * Just use getattr * Make sure we have anymail extras Currently just ses, mailgun, and sendgrid. We can add more easily. * Enable all the different AnyMail backends * Merge `mail` app into `admin` app * Swap to `EmailMultiAlternatives` * Remove some unused variables * Add SMTP settings * Replace references to settings.MAIL["SEND"] Now we assume email is disabled if `EMAIL_PROVIDER` is unset, and send emails to `django.core.mail.backends.dummy.EmailBackend` instead. * Fix AWS variable name * Revert change to Docker Compose * Remove random newline * Bring back the image in emails * Default mail provider in dev to console * Sendgrid is no longer a dependency * Swap email for loop to empty * Force email_enabled to false in testing * Issue users tokens even when mail is disabled How was this in the code base before... * Change default outbound email Feels like we should alert the user somehow if they've not set this * Don't break email verification * Catch SMTP errors or any errors from AnyMail * Fix email enabled syntax in tests

thebeanogamer added 4 commits October 30, 2021 15:22

Add Django Anymail

9e50020

Don't just disable email, use Django's builtin

79b2d55

Add a page showing sent emails

97f7862

This doesn't work yet, as it depends on Django's builtin email stuff. That'll come next

Remove all our custom email code

2180cd1

thebeanogamer added 7 commits October 30, 2021 19:05

Actually install the Mail app

c6208b7

Remove unused tests file

462040c

Remove now-needed code from staging.py

1b4b1c7

Remove dumb import catching

fe7e8ad

Remove unused imports

33d0cbf

Just use getattr

7cfa29a

Make sure we have anymail extras

31caf53

Currently just ses, mailgun, and sendgrid. We can add more easily.

0xAda requested changes Oct 30, 2021

View reviewed changes

thebeanogamer added 5 commits October 30, 2021 22:00

Enable all the different AnyMail backends

bef844d

Merge mail app into admin app

ed6598f

Swap to EmailMultiAlternatives

a85cd4d

Remove some unused variables

64c0beb

Add SMTP settings

a4a6ae5

0xAda reviewed Oct 30, 2021

View reviewed changes

docker-compose.yml Outdated Show resolved Hide resolved

Replace references to settings.MAIL["SEND"]

30eb4fe

Now we assume email is disabled if `EMAIL_PROVIDER` is unset, and send emails to `django.core.mail.backends.dummy.EmailBackend` instead.

thebeanogamer force-pushed the feature/mail-refactor branch from e2edffc to 30eb4fe Compare October 30, 2021 23:17

thebeanogamer added 3 commits October 31, 2021 00:49

Fix AWS variable name

feb2762

Revert change to Docker Compose

6b1e8fd

Remove random newline

89ec0ec

Bring back the image in emails

089bfa3

Issue users tokens even when mail is disabled

5ab5e9f

How was this in the code base before...

Change default outbound email

b43b521

Feels like we should alert the user somehow if they've not set this

thebeanogamer requested a review from 0xAda December 11, 2021 16:19

0xAda reviewed Dec 11, 2021

View reviewed changes

src/authentication/serializers.py Outdated Show resolved Hide resolved

Don't break email verification

1344b8f

thebeanogamer requested a review from 0xAda December 11, 2021 16:37

0xAda approved these changes Dec 11, 2021

View reviewed changes

jchristgit requested review from jchristgit and Bentechy66 January 9, 2022 23:53

jchristgit self-assigned this Jan 9, 2022

jchristgit reviewed Jan 11, 2022

View reviewed changes

jchristgit approved these changes Jan 11, 2022

View reviewed changes

jchristgit assigned 0xAda and jchristgit and unassigned jchristgit Jan 14, 2022

jchristgit added the enhancement New feature or request label Jan 15, 2022

Catch SMTP errors or any errors from AnyMail

ce3e138

Fix email enabled syntax in tests

791c9e9

jchristgit approved these changes Jan 16, 2022

View reviewed changes

jchristgit merged commit b383bdb into master Jan 16, 2022

jchristgit deleted the feature/mail-refactor branch January 16, 2022 17:07

jchristgit assigned thebeanogamer and unassigned 0xAda and jchristgit Jan 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use Django's Email Functionality #220

Use Django's Email Functionality #220

Uh oh!

thebeanogamer commented Oct 30, 2021 •

edited

Loading

Uh oh!

codecov bot commented Oct 30, 2021 •

edited

Loading

Uh oh!

0xAda left a comment

Uh oh!

thebeanogamer commented Oct 30, 2021

Uh oh!

Uh oh!

thebeanogamer commented Oct 31, 2021

Uh oh!

404dcd commented Oct 31, 2021

Uh oh!

thebeanogamer commented Oct 31, 2021

Uh oh!

thebeanogamer commented Oct 31, 2021

Uh oh!

thebeanogamer commented Dec 11, 2021 •

edited

Loading

Uh oh!

Uh oh!

0xAda left a comment

Uh oh!

jchristgit Jan 11, 2022

Uh oh!

jchristgit left a comment

Uh oh!

jchristgit commented Jan 11, 2022

Uh oh!

thebeanogamer commented Jan 15, 2022 •

edited

Loading

Uh oh!

jchristgit left a comment

Uh oh!

Uh oh!

Use Django's Email Functionality #220

Use Django's Email Functionality #220

Uh oh!

Conversation

thebeanogamer commented Oct 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

0xAda left a comment

Choose a reason for hiding this comment

Uh oh!

thebeanogamer commented Oct 30, 2021

Uh oh!

Uh oh!

thebeanogamer commented Oct 31, 2021

Uh oh!

404dcd commented Oct 31, 2021

Uh oh!

thebeanogamer commented Oct 31, 2021

Uh oh!

thebeanogamer commented Oct 31, 2021

Uh oh!

thebeanogamer commented Dec 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

0xAda left a comment

Choose a reason for hiding this comment

Uh oh!

jchristgit Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

jchristgit left a comment

Choose a reason for hiding this comment

Uh oh!

jchristgit commented Jan 11, 2022

Uh oh!

thebeanogamer commented Jan 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jchristgit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thebeanogamer commented Oct 30, 2021 •

edited

Loading

codecov bot commented Oct 30, 2021 •

edited

Loading

thebeanogamer commented Dec 11, 2021 •

edited

Loading

thebeanogamer commented Jan 15, 2022 •

edited

Loading