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.

Sign up for GitHub

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

Separate LOGIN_FIELD logic into a auth backend to avoid giving tokens… #810

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+46 −4
Open

Separate LOGIN_FIELD logic into a auth backend to avoid giving tokens… #810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
172e307
Separate LOGIN_FIELD logic into a auth backend to avoid giving tokens…
Apr 5, 2024
7cb7cb8
Merge branch 'master' into master
tomwojcik Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions djoser/backends.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
from django.contrib.auth import get_user_model
from django.contrib.auth.models import Permission
from django.contrib.auth.backends import ModelBackend
from django.db.models import Exists, OuterRef, Q
from djoser.conf import settings

User = get_user_model()


class LoginFieldBackend(ModelBackend):

def authenticate(self, request, username=None, password=None, **kwargs):
if username is None:
username = kwargs.get(settings.LOGIN_FIELD)
if username is None or password is None:
return

Check warning on line 16 in djoser/backends.py

View check run for this annotation

Codecov / codecov/patch

djoser/backends.py#L16 

Added line #L16 was not covered by tests
user = User.objects.filter(**kwargs).first()
if user and not user.check_password(password):
return

Check warning on line 19 in djoser/backends.py

View check run for this annotation

Codecov / codecov/patch

djoser/backends.py#L19 

Added line #L19 was not covered by tests
if user and user.is_active:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good to have so djoser is compatible with Django's default auth model backend. However to be fully covered in the future I would either try to call super().authenticate() or at least self.user_can_authenticate().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your returns. This approach is a simple copy-paste of the current behaviour of djoser, but exported to a custom Auth backend.

The origin of this piece of code is the availability of using a LOGIN_FIELD without a CustomUserModel( #389 and 8f65bff).

My idea is to keep current djoser behavior for people using this way of declaring user model in djoser config and expecting working like today by using a custom auth backend.
All other people expecting standard django behaviour will have nothing to do. Remember that this custom backend will be in a list with other auth backends, so, if the dev includes the django default backend, both login behaviours (default django one, and custom djoser) will be taken in account.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining your point of view! From that perspective it makes some sense it keep it more compatible with current behaviour.

However with any next major release it would be good to use the default Django's auth backend behaviour and implementation.

@tomwojcik would you merge this PR to the upcoming 2.x version or would you wait until version 3? I think I would go for upcoming 2.x with proper documentation in the changelog.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd definitely release the fix in 2.3 as it's a minor security issue, but at the same time I'm not sure if these particular changes are actually needed.

IMO the changes from 8f65bff are incorrect. If the user didn't pass authenticate, then they shouldn't be able to login.

If you need a custom behavior, it's up to the ModelBackend and user manager implementation (like @hisie did). Handling it by Djoser is a shortcut.

return user
4 changes: 0 additions & 4 deletions djoser/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,6 @@ def validate(self, attrs):
self.user = authenticate(
request=self.context.get("request"), **params, password=password
)
if not self.user:
self.user = User.objects.filter(**params).first()
if self.user and not self.user.check_password(password):
self.fail("invalid_credentials")
if self.user and self.user.is_active:
return attrs
self.fail("invalid_credentials")
Expand Down
2 changes: 2 additions & 0 deletions docs/source/settings.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ custom User model.

**Default**: ``User.USERNAME_FIELD`` where ``User`` is the model set with Django's setting AUTH_USER_MODEL.

To use this feature you should add "djoser.backends.LoginFieldBackend" to your AUTHENTICATION_BACKENDS at the last position.

.. warning::

Djoser uses `djangorestframework-simplejwt`_ to provide a convenient integration for JWT.
Expand Down
23 changes: 23 additions & 0 deletions testproject/testapp/tests/test_token_create.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,13 @@ def test_post_should_not_login_if_empty_request(self):
)

@override_settings(DJOSER=dict(django_settings.DJOSER, **{"LOGIN_FIELD": "email"}))
@override_settings(AUTHENTICATION_BACKENDS=[
"django.contrib.auth.backends.ModelBackend",
"djoser.social.backends.facebook.FacebookOAuth2Override",
"social_core.backends.google.GoogleOAuth2",
"social_core.backends.steam.SteamOpenId",
"djoser.backends.LoginFieldBackend",
])
def test_login_using_email(self):
user = create_user()
previous_last_login = user.last_login
Expand All @@ -93,3 +100,19 @@ def test_login_using_email(self):
self.assertEqual(response.data["auth_token"], user.auth_token.key)
self.assertNotEqual(user.last_login, previous_last_login)
self.assertTrue(self.signal_sent)

@override_settings(DJOSER=dict(django_settings.DJOSER, **{"LOGIN_FIELD": "email"}))
def test_login_using_email_without_login_backend(self):
user = create_user()
previous_last_login = user.last_login
data = {"username": user.email, "password": user.raw_password}
user_logged_in.connect(self.signal_receiver)

response = self.client.post(self.base_url, data)
user.refresh_from_db()

self.assert_status_equal(response, status.HTTP_400_BAD_REQUEST)
self.assertEqual(
response.data["non_field_errors"],
[settings.CONSTANTS.messages.INVALID_CREDENTIALS_ERROR],
)
Loading