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

Disallow custom authentication class to return user = None #9574

Open
4 of 6 tasks
leandrodesouzadev opened this issue Oct 23, 2024 · 2 comments
Open
4 of 6 tasks

Disallow custom authentication class to return user = None #9574

leandrodesouzadev opened this issue Oct 23, 2024 · 2 comments

Comments

@leandrodesouzadev
Copy link

leandrodesouzadev commented Oct 23, 2024

Hello there!
Currently it's allowed that an custom authentication class returns a tuple containing (None, None). This causes the authentication to be considered successful and sets request.user = None and this can cause errors that are hard to track where they came from.
I know that this wrongly implemented by the CustomAuthentication class, the user should've raised an AuthenticationFailed exception instead. But this allows the user to shoot itself on the foot.

Example of a bad implemented authentication class:

from rest_framework.authentication import BaseAuthentication

class MyDumbAuthentication(BaseAuthentication):
  def authenticate(self, request):
    return None, None

Later if you have a permission check for example, you would see the following error:

from rest_framework.permissions import BasePermission

class MyPermCheck(BasePermission):
  def has_permission(self, request, view):
    return request.user.has_perm("foo.bar")

# raises AttributeError("'NoneType' object has no attribute 'has_perm'")

It would be nice if rest framework disallowed this totally wrong implementation.

Checklist

  • Raised initially as discussion
  • This is not a feature request suitable for implementation outside this project. Please elaborate what it is:
    • compatibility fix for new Django/Python version ...
    • other type of bug fix
    • other type of improvement that does not touch existing code or change existing behavior (e.g. wrapper for new Django field)
  • I have reduced the issue to the simplest possible case.
@leandrodesouzadev
Copy link
Author

I would be happy to submit a patch to disallow this!

@krishnamadhavan
Copy link

krishnamadhavan commented Oct 26, 2024

@leandrodesouzadev All implementations of permission_classes from rest_framework uses something like below:

return bool(request.user and request.user.is_staff)

So, you always verify request.user before calling any methods on request.user.

Alternatively, like here, you can raise errors before returning from the function. This way we ensure we never return None.

Does that answer your question? Happy to help otherwise.

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

No branches or pull requests

2 participants