search

iMerica / dj-rest-auth

Authentication for Django Rest Framework
https://dj-rest-auth.readthedocs.io/en/latest/index.html
MIT License
1.63k stars 304 forks source link

Fixed Incompatibility with django-allauth v0.55.0 #536

Closed brandon-kong closed 10 months ago

brandon-kong commented 11 months ago

dj-rest-auth is not compatible with django-allauth's latest version. This PR fixes that issue upon the newest release of django-allauth v0.55.0.

I created a email_address_exists method in dj_rest_auth's utils with the same implementation.

flange-ipb commented 11 months ago

Hi @brandon-kong, for the sake of maintainability, wouldn't it be better to adopt the drop-in replacement from django-allauth? If you look at commit 9fccdd0 email_address_exists is replaced by EmailAddress.objects.is_verified.

brandon-kong commented 11 months ago

Hi @brandon-kong, for the sake of maintainability, wouldn't it be better to adopt the drop-in replacement from django-allauth? If you look at commit 9fccdd0 email_address_exists is replaced by EmailAddress.objects.is_verified.

Good catch, I completely missed their new replacement.

brandon-kong commented 11 months ago

Hi @brandon-kong, for the sake of maintainability, wouldn't it be better to adopt the drop-in replacement from django-allauth? If you look at commit 9fccdd0 email_address_exists is replaced by EmailAddress.objects.is_verified.

Actually, is that the intended use-case? As I'm testing it now, users would be able to create accounts with already-existing emails if the email is not verified; I assume that isn't the intended result. Wouldn't it make more sense to filter for emails existing rather than filtering for verified existing emails? I'll revert my code back to the previous commit I pushed

Dresdn commented 11 months ago

I had started to work on this, and then after seeing your direction, I decided just to commit and opened #539.

Actually, is that the intended use-case? As I'm testing it now, users would be able to create accounts with already-existing emails if the email is not verified; I assume that isn't the intended result. Wouldn't it make more sense to filter for emails existing rather than filtering for verified existing emails? I'll revert my code back to the previous commit I pushed

I disagree and think that is the intended result. With the method you outlined, I could mass register all emails and effectively block users from signing up. Instead, you should allow duplicate email registrations, as only one will actually get verified. Then, you can run a cleanup job to remove all verified=False objects after a certain amount of time.