sunscrapers / djoser

REST implementation of Django authentication system.
MIT License
2.55k stars 458 forks source link

Duplicated email address leads to 500 error #716

Open zvolsky opened 1 year ago

zvolsky commented 1 year ago

I think 2+ users can share same e-mail address. However when 2+ users with same address are inactive, djoser will fail so there is no possibility to activate such users. (And this is danger, because the user can try add next usernames to make the account with such email working.)

As you can see, only Django, Rest_framework & Djoser are in the traceback, so I think Djoser should be fixed.

The problem is in the serializers.py, class UserFunctionsMixin, def get_user() where the orm call User._default_manager.get() is handled for User.DoesNotExist but not for User.MultipleObjectsReturned.

I think instead of .get() we could use .filter().first() here (with appropriate removal of try/except). This would make the user activating possible, the 1st one first, then the next..

Of course the users identification by email and not by username is not good here. However I think such solution could give some improvement still.

The other question, which I am not able to answer now, is: Is it possible to fix it in this way for all scenarios where UserFunctionsMixin is used?

Internal Server Error: /api/v1/users/resend_activation/
Traceback (most recent call last):
  File "/home/mirek/dj/authtemplate/authtemplate/.venv/lib/python3.10/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
  File "/home/mirek/dj/authtemplate/authtemplate/.venv/lib/python3.10/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/home/mirek/dj/authtemplate/authtemplate/.venv/lib/python3.10/site-packages/sentry_sdk/integrations/django/views.py", line 85, in sentry_wrapped_callback
    return callback(request, *args, **kwargs)
  File "/home/mirek/dj/authtemplate/authtemplate/.venv/lib/python3.10/site-packages/django/views/decorators/csrf.py", line 54, in wrapped_view
    return view_func(*args, **kwargs)
  File "/home/mirek/dj/authtemplate/authtemplate/.venv/lib/python3.10/site-packages/rest_framework/viewsets.py", line 125, in view
    return self.dispatch(request, *args, **kwargs)
  File "/home/mirek/dj/authtemplate/authtemplate/.venv/lib/python3.10/site-packages/rest_framework/views.py", line 509, in dispatch
    response = self.handle_exception(exc)
  File "/home/mirek/dj/authtemplate/authtemplate/.venv/lib/python3.10/site-packages/rest_framework/views.py", line 469, in handle_exception
    self.raise_uncaught_exception(exc)
  File "/home/mirek/dj/authtemplate/authtemplate/.venv/lib/python3.10/site-packages/rest_framework/views.py", line 480, in raise_uncaught_exception
    raise exc
  File "/home/mirek/dj/authtemplate/authtemplate/.venv/lib/python3.10/site-packages/rest_framework/views.py", line 506, in dispatch
    response = handler(request, *args, **kwargs)
  File "/home/mirek/dj/authtemplate/authtemplate/.venv/lib/python3.10/site-packages/djoser/views.py", line 202, in resend_activation
    user = serializer.get_user(is_active=False)
  File "/home/mirek/dj/authtemplate/authtemplate/.venv/lib/python3.10/site-packages/djoser/serializers.py", line 132, in get_user
    user = User._default_manager.get(
  File "/home/mirek/dj/authtemplate/authtemplate/.venv/lib/python3.10/site-packages/django/db/models/manager.py", line 85, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/mirek/dj/authtemplate/authtemplate/.venv/lib/python3.10/site-packages/django/db/models/query.py", line 653, in get
    raise self.model.MultipleObjectsReturned(
apps.core.models.User.MultipleObjectsReturned: get() returned more than one User -- it returned 4!
[09/Feb/2023 16:28:55] "POST /api/v1/users/resend_activation/ HTTP/1.1" 500 117478
zvolsky commented 1 year ago

I have not tested it but I think if we will allow duplicate emails, not only Activation (with possible solution above), but more actions will have problem. So probably a better approach would be to disable duplicate emails. Soemthing as is described here. https://stackoverflow.com/questions/55969952/how-can-i-avoid-a-user-from-registering-an-already-used-email-in-django. Maybe this can be applied without Djoser's change, if we would inherit UserViewSet and add email validation for User Create.

.. if so, this can be closed; but I let it open, because I am curious for meaning of others.

tomwojcik commented 1 year ago

Thanks for creating the ticket, you're right. PRs welcome!

tomwojcik commented 1 year ago

I have found some time to have a look into that. This integrity check should be implemented in the DB with a unique constraint.

If such index existed, the create serializer will prevent from creating a duplicate user.

https://github.com/sunscrapers/djoser/blob/0cef72f2e62f1d9e821d49814cd857c098371b7b/djoser/serializers.py#L37-L44

But I agree, this could be a useful feature in Djoser as well.

tomwojcik commented 1 year ago

I closed the PR above as we can't justify an additional query to the DB to meet the criteria of a few. I will leave this ticket open. If someone else has a better idea, let me know.

Depending on the project setup and requirements, it may or may not be a bug.