feat(accounts): Add notifications for account events

[varunsaral]

This PR will close #3411

[varunsaral]

@pennersr Are we looking for something along this lines for account related notifications?

[coveralls]

coverage: 95.835% (+0.02%) from 95.819%
when pulling 237aedf on varunsaral:send_notification_email
into 4982485 on pennersr:main.

[pennersr]

@pennersr Are we looking for something along this lines for account related notifications?

Yes, definitely going in the right direction. Thanks for picking this up. I'll add some preliminary review comments...

[varunsaral]

@pennersr I made changes according to the review, please have a look if they are fine so i can do changes for other events too.

[varunsaral]

Hello @pennersr, I have made chanegs along with testcases, can you please give it a review so i can replicate it for other events, currently it is implemented for password reset.

[varunsaral]

@pennersr sorry if I'm asking for too much reviews but can I move forward with the implementation?

[pennersr]

@pennersr sorry if I'm asking for too much reviews but can I move forward with the implementation?

No need to be sorry -- sorry I am not able to always provide a timely response. In any case, don't let that stop you from moving forward, as mentioned, this is starting to shape up quite nicely, and the review comments are really only minor things that also can be picked up at a later time.. 👍

[varunsaral]

Here's how the current email looks, it is looking fine to me.

[varunsaral]

Hello @pennersr, just cheking in for update. Can you please give it a quick review and give me more places to add notification. Also i am having little difficulties adding test for change_email (_add_primary) please help me out in this.

[pennersr]

Hello @pennersr, just cheking in for update. Can you please give it a quick review and give me more places to add notification. Also i am having little difficulties adding test for change_email (_add_primary) please help me out in this.

In test_change_email.py there is e.g. a test case called test_add() which tests adding an email. You can add mailoutbox to the list of its parameters, and then inspect that outbox to see that there 2 emails in there:

• One verification mail
• The second (to the current primary) -- your account security email.

As for places where these notifications should appear:

• Password change
• Social account added/removed
• Email added/removed, primary changed
• MFA enabled/disabled
• MFA new recovery codes generated

Hope that helps.

[dwasyl]

@varunsaral @pennersr Just a thought and perhaps out of scope for this PR at this point, but it seems like it would make the most sense to match these e-mails to the signals that get sent. I believe all the original functions send signals for those same events, but would it be possible to add matching MFA signals? (Although I might add MFA recovery code used or MFA recovery code viewed/downloaded since pretty much anything regarding MFA may be notification worthy).

Further, would it be possible to configure which events send notification e-mails rather than it being an all or nothing configuration?

[varunsaral]

@dwasyl I think we should have mfa signals, and @pennersr can better tell why we don't have them already. I can probably add them if required. Morever for notification customization, if we have a strong usecase then we can add it. We can also add them in category for example, simple account, social accounts and mfa related notification.

[pennersr]

@dwasyl I think we should have mfa signals, and @pennersr can better tell why we don't have them already.

They are there now: 108f3cd

[varunsaral]

Bravo 👏 that was very fast @pennersr , i was already planning how to add them 🙂. Can you also please tell about the suggestion of adding option to customise notifications events

[dwasyl]

They are there now: 108f3cd

Ha that was very quick! That's great, I know it's not the place to ask but just since it's related... Any chance of additional few signals for when viewing/downloading recovery codes? Or when there's a success/fail mfa authentication (if lots of failures might indicate the initial password was compromised)?

[varunsaral]

Hello @pennersr , we don't send emails from social app. Should we reuse from accounts app or should we create same thing in social accounts?

[varunsaral]

@pennersr Work is almost complete, notification has been added, just have to add few tests around socialaccounts and mfa which are little complex for now. If time allows then give it a review. Thanks

[varunsaral]

@pennersr This MR is complete(just 2 testcases remains) and this can be reviewed now. Don't know why checks are failing.

[varunsaral]

@pennersr fixed according to the suggestions

[varunsaral]

@pennersr This one is pending for some reviews, i made changes according to the suggestions . Please give it a quick review when possible.

[varunsaral]

@pennersr All changes made according to suggestions.

[varunsaral]

@pennersr all merge conflict has been resolved and all changes made according to the suggestions, this can be merged if we don't have any further changes.

[pennersr]

I gave the changes a rundown. In doing so, I rebased and made a few tweaks, most notably avoiding duplicating the test logic, as well as several textual changes. I pushed this on a branch here: https://github.com/pennersr/django-allauth/tree/feat-security-notifications

I think we need to look into these bits still:

• We need to pass the provider to the socialaccount (dis)connected notifications.
• When changing email from A to B, we actually need to send the notification to the original email (A). Email B is already aware of the change, as it received an email confirmation message

[pennersr]

(No need to take action here -- I am looking into this as we speak...)

[pennersr]

Merged via #3591 -- thanks!

[varunsaral]

Thanks @pennersr, seems like you had to do major rework for it to get merged. I still have many things to learn.