sunscrapers / djoser

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

DJOSER is giving tokens to users that don't pass the AUTHENTICATION_BACKENDS.authenticate #795

Closed hisie closed 1 week ago

hisie commented 9 months ago

https://github.com/sunscrapers/djoser/blob/4452009f628b59f02704373d5e7f991f1243397f/djoser/serializers.py#L127

When retrieving a token DJOSER check itself if the password match, or not. If password match, only active or not validation is done.

Django authentication backends propose a set of method to do authentication centralised by backend, while respecting the configuration order (first login accepted first groups and permissions given to the user).

django.contrib.auth.authenticate is used in AUTHENTICATION_BACKENDShttps://github.com/sunscrapers/djoser/blob/4452009f628b59f02704373d5e7f991f1243397f/djoser/serializers.py#L122. This function call, in the order declared in AUTHENTICATION_BACKENDS, the authenticate method for each backend.

After checking if the login is correct in all authentication backends, if one user without any permission to get token has a user -password that match, DJOSE give the token.

DJOSER should accept the result of authenticate because developpers can add more condition in the backends to allow user be authenticated.

hisie commented 9 months ago

After reading the doc ( https://djoser.readthedocs.io/en/latest/settings.html#login-field) I found this setting an overkill feature. If django user model allows setting the login-field, overriding this with another layer while ignoring the authenticate method can give more problems than solutions.

tomwojcik commented 7 months ago

After reading the doc ( https://djoser.readthedocs.io/en/latest/settings.html#login-field) I found this setting an overkill feature. If django user model allows setting the login-field, overriding this with another layer while ignoring the authenticate method can give more problems than solutions.

It literally works the same as Django, as the default of LOGIN_FIELD is USERNAME_FIELD. I don't understand what your concern is.

Both the title and the OG msg make it sound as if there was a security issue in Djoser. Djoser does NOT allow users to obtain tokens, if they did not pass the authenticate check.

Django's default behavior is to pass the provided credentials through all authentication backends until one succeeds. Once a backend successfully authenticates the user, Django doesn't continue to check other backends.

hisie commented 7 months ago

Djoser does NOT allow users to obtain tokens, if they did not pass the authenticate check.

This is not true: I have tokens sent to users not passing the only valid authenticate in my configuration:

I'm going to show the code. Maybe I'm doing something wrong: Backend authentication configuration: AUTHENTICATION_BACKENDS = ["user_profile.auth_backend.MyCustomBackendAuthentication"] Backend authentication:

from django.contrib.auth.backends import ModelBackend

class MyCustomBackendAuthentication(ModelBackend):
    def user_can_authenticate(self, user):
        """
        Reject users with is_active=False or are unsuscribed
        """
        return super().user_can_authenticate(user) and not user.is_unsuscribed()

Use case:

I have an active user that has unsubscribe to my service. user.is_unsuscribed() is false, so authenticate (https://github.com/django/django/blob/main/django/contrib/auth/backends.py#L48) is false because of user_can_authenticate is false. He logs in giving correct user and password. My authentication backend says "No, you are unsubscribed, you should not log in", authenticate method (inherited from ModelBackend) says no because of "user_can_authenticate". I have only one authentication backend, so this is a definitely "NO".

Expected behaviour:

Obtained behaviour:

Sorry for the confusion about the LOGIN_FIELD. I thought it was related to the problem I'm having.

haxoza commented 7 months ago

Thank you @hisie for submitting it and defending your point!

After a brief analysis I think you're right. Djoser should not bypass django's auth backends.

It seems the issue is related to issue #389 and commit 8f65bff

hisie commented 7 months ago

Hello @haxoza and @tomwojcik . The ticket was set as invalid before my last answer and after Haxoza comment saying issue and related code is still invalid. I have done a proposal to keep old behaviour by adding a django auth backend. This allows:

Please, tell me if you need anything more to get my pull request acepted. I have not found a PR guide or protocol.

Thank you.

tomwojcik commented 6 months ago

Thank you @hisie for providing more info. After adding more context, I agree with everything you said. Lets continue the discussion in the PR.

tomwojcik commented 1 week ago

Thanks again, released in 2.3.0.