pennersr / django-allauth

Integrated set of Django applications addressing authentication, registration, account management as well as 3rd party (social) account authentication.
https://allauth.org
MIT License
9.56k stars 3.04k forks source link

Modify EmailConfirmationHMAC.confirm() and EmailConfirmation.confirm() methods to emit the email_confirmed signals for already verified emails #2857

Closed arnav13081994 closed 2 years ago

arnav13081994 commented 3 years ago

Due to how the EmailConfirmationHMAC.confirm() and the EmailConfirmation.confirm() methods are implemented, there is no easy way to ensure that an email that was verified in the past was again verified. Not even a post_save receiver on the EmailAddress model can solve that because both the email_confirmed signal and the Actual Model instance getting updated and saved only happen if the emailaddress in question has not already been confirmed.

In my use case, I need to verify emails again regardless of whether they may already have been verified in the past. This is due to security considerations. We can only give access to Restricted parts of the website only after re-verifying the emails This is due to a bunch of reasons with the most common concern being that one can leave an organization and still access the Restricted parts if they already verified their email while they were at their previous job.

I propose to remove this one line

if not self.email_address.verified:

from the EmailConfirmationHMAC.confirm() and the EmailConfirmation.confirm() methods. Doing so would have the following advantages:

1) email_confirmed signals would always be emitted for when the email is verified regardless of whether it was verified in the past or not. This will support all use-cases involving verifying an already verified email address. Will also be useful if someone wants to trigger re-verifying all emails every couple of months or so or before giving a user access to some restricted part of the website.

2) post_save signals on the EmailAddress model would also get emitted.

3) This should also take care of cases in which the Social Provider adds an email with verified=True but one still needs to verify the email again. Perhaps for the same reasons listed in 1). Making this change will easily fix this issue as well.

9mido commented 3 years ago

I see your use case but when it comes to dealing with signals and allauth social accounts I am not the right person for dealing with that.

I think you can do this with a custom management command and cronjob and maybe a different form besides re-send verification. Maybe you could use OTP instead of email re-verification. Perhaps make a separate table / model in your app and update the values in that new table / model to allow / deny access to parts of your site. This new database table is to keep a separate record if users are verified again (you can even add a date field if you want). You may or may not need to update the allauth values from verified to unverified and vice versa once they re-verify again. If they don't verify within a certain amount of days after being unverified, you can do something to reflect that like make them inactive.

Maybe use a custom user model and update existing database columns or create new ones. There is the 'active' column and the 'staff' column. You would just have to change update those values.

Just from a UX perspective, I think users would be annoyed thinking they have to verify again when they already did it in the past. The only benefit would be for you as the site owner to ensure that the user is truly a legit user and not some bot. Most users probably won't see any security benefits and only you will.

The purpose of PR 2770 is to only let unverified emails verify. It seems wasteful, spammy to your email provider, and annoying to your users to verify their already verified emails again.

If you come up with a solution that won't interfere with PR 2770 then feel free to share it in this comment thread or make another PR. The best I can do for now is to make a setting in PR 2770 so that you can enable / disable re-sending emails to all users or unverified users only. I added this setting for you so hopefully you can still do what you need to do.

arnav13081994 commented 3 years ago

Thank you for your prompt and thoughtful answer.

I really appreciate you pushing that fix for me, but unfortunately my use-case is custom enough that I ended up overriding the ConfirmEmailView class' post() method to make sure email_confirmed signals were getting fired. The other main reason was to be able to access some metadata that I was sending.

Using the same Email Verification flow has created some interesting problems regarding figuring out which 'action' the now verified emailaddress belongs to, if that makes sense. Because in my use-case it is not just limited to new users signing up, but also re-verifying existing users as well as allowing existing users to 'upgrade', which also prompts a verification. In this scenario, it could be a new email or an existing email (one could use their personal emails too.)

I think playing around with is_active on the User table might be interesting to do and definitely something I'd explore. Thank you so much once again for you help and suggestions.

9mido commented 3 years ago

You're welcome. Maybe share more of your code so far so others can be helped also and you could be helped more.

pennersr commented 2 years ago

It seems to be that you are using the email confirmation mechanism for a different purpose. To make this work for you, tackling that if statement is not sufficient.

For example, you would also need to change the wording of the email verification mail itself, to explain that this mail is sent because of an extra security step instead of a user adding an email to an account for signing in. All in all, this is not just a matter of that if. And, I assume that in order to make this work you need all sorts of additional logic, like remember when the last email verification happened to see if access to the restricted parts of the site is still allowed.

I think it is warranted to introduce a (project specific) mechanism specifically tailored to your needs instead of trying to shoehorn the email verification functionality of allauth...