Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicated email address leads to 500 error #716

Open
zvolsky opened this issue Feb 9, 2023 · 4 comments
Open

Duplicated email address leads to 500 error #716

zvolsky opened this issue Feb 9, 2023 · 4 comments

Comments

@zvolsky
Copy link

zvolsky commented Feb 9, 2023

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
Copy link
Author

zvolsky commented Feb 10, 2023

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
Copy link
Contributor

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

@tomwojcik tomwojcik added this to the 2.2.1 milestone May 20, 2023
@tomwojcik
Copy link
Contributor

tomwojcik commented Jul 2, 2023

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.

class UserCreateMixin:
def create(self, validated_data):
try:
user = self.perform_create(validated_data)
except IntegrityError:
self.fail("cannot_create_user")
return user

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

@tomwojcik
Copy link
Contributor

tomwojcik commented Nov 5, 2023

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.

@tomwojcik tomwojcik removed this from the 2.2.1 milestone Nov 5, 2023
@tomwojcik tomwojcik removed the bug label Nov 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants