feat(preferences): added personal email verification #1781
Conversation
|
Also, how are periodic celery tasks defined? I tried defining one programmatically to delete all expired unverified emails but no other tasks are seemed to be defined this way. I trying looking through the admin panel, but I still wasn't able to find where I could define a periodic celery-beat task. |
|
I think this is what you're looking for ion/intranet/settings/__init__.py Line 923 in 0ad6651 |
|
Ah I see, thank you. |
c9824af to
f9fa744
Compare
|
@aarushtools is something like this what you had in mind? |
|
yeah that looks good |
|
Alright, I amended the commit with the new template |
There was a problem hiding this comment.
This looks really good! Just one thing:
The problem with using a celery task to delete unverified emails is that the links can stay active for longer than 12 hours. Since the celery task is set run at midnight, you could send an email out at 3 PM and it would be active for 9 hours before being cleared (instead of the expected 6 hours).
I suggest to see if the email link is supposed to be expired when the link is accessed in the verify_email_view, and if so, redirect to a page telling the user the link has expired (not Http404). Keep the celery task though just to make sure there isn't a buildup of links that were never clicked.
Also, can you make the max time to verify before the link expires 12 hours (in addition to making it a setting in settings/__init__.py).
|
Thanks for the review! I'll make a new template informing the user that a link is expired, not just throwing a 404 if it is. |
|
Here's an idea for the verification page: I think it would be nice if it redirected to the preferences page, and then just sent a message saying "Your email has been verified, you can add it as a primary email now." (Django message in the bottom right) So then everything is in one place and we don't have to deal with making a new verified page. |
|
That sounds good, it would be a lot cleaner. I'll work on it right now. |
0c93630 to
a1d0c30
Compare
|
I made a mistake trying to resolve some conflicts, I'll try to have my changes finished sometimes this week. |
27d56f1 to
7e57a17
Compare
|
The amount of time it takes for an verification link to expire has been parameterized in |
|
Aarush will review this, but another request: could you add this to the senior email forwarding (emailfwd) app? |
|
Sure, I'll work on it this weekend. |
|
I've worked on and given this implementation some thought, and I think the best approach to adding email verification to the senior emailfwd app would be to have the user select from the already verified email addresses in the preferences app. This way, we wouldn't have to duplicate the verification logic and keep all email verification centralized in a single app. I think this would make the system simpler and more maintainable. |
|
Yes, that makes sense. So could you do the following then:
|
|
Yes that all sounds good, I'll continue working on it. |
|
Alright, the emailfwd app only accepts emails verified in the preferences app now.
|
|
@aarushtools Could you review this when you get the chance? |
Closes #1739
Proposed changes
UnverifiedEmailmodel with a foreign key to the email and a UUID field for the verification link.preferences/verify_email/<uuid>/) to verify the email and associated it with the user and unique UUID.verifiedboolean field to theEmailmodel.Brief description of rationale
When a user adds a new personal email in preferences, Ion should verify that they actually own the email. Ion should send a verification link containing a link, that, when clicked on, confirms the email ownership. This prevents users from adding addresses that are incorrect or that they do not own.
I have tested manually inputting the UUID in the url, and the logic does correctly verify the email. Also, it doesn't allow selecting non verified email as the primary notification email. The actual sending of the email should be tested extensively as I cannot test it in a development environment.