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

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

Closed
hisie opened this issue Feb 16, 2024 · 7 comments · Fixed by #819
Closed
Assignees

Comments

@hisie
Copy link

hisie commented Feb 16, 2024

if self.user and not self.user.check_password(password):

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 hisie changed the title DJOSER is giving tokens to inactive users and user that don't have the AUTHENTICATION_BACKENDS.user_can_authenticate DJOSER is giving tokens to users that don't pass the AUTHENTICATION_BACKENDS.authenticate Feb 16, 2024
@hisie
Copy link
Author

hisie commented Feb 16, 2024

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

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

hisie commented Apr 1, 2024

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:

  • DJOSER don't give this user any token, because the only allowed backend says no.

Obtained behaviour:

  • DJOSER, knowing that this user has no permission to login (
    self.user = authenticate(
    returns false because of the custom backend), its do an additional check after the reject from the backend (
    if self.user and not self.user.check_password(password):
    ) by simply checking if the user exists (it exists) and if the password is fine( the password is fine) to let an unauthorized user get a user token.

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

@tomwojcik tomwojcik reopened this Apr 1, 2024
@haxoza
Copy link
Member

haxoza commented Apr 3, 2024

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

hisie commented Apr 10, 2024

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:

  • People using current behaviour, easily add the auth backend to keep it working,
  • People wanting more restricted behaviour, get expected login behaviour by default.
    I have added tests to check no bypass is done in default configuration, and to check that custom auth backend works as expected (current behaviour)

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 tomwojcik removed the invalid label May 3, 2024
@tomwojcik
Copy link
Contributor

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

Thanks again, released in 2.3.0.

@tomwojcik tomwojcik self-assigned this Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants