assert isinstance(request, HttpRequest) prevents use with Django Rest Framework (drf) about django-pam HOT 9 CLOSED

If you look at the code you'll see that the request argument is positional meaning something must be in that position. The assert allows a None or a valid request object so the calling code must put something there in any case.

With the above said I've used this with DRF with no problems. What version of DRF are you using and what in DRF is calling the backend? Also, what version of django-pam are you using?

from django-pam.

Closing Issue, no bug found.

from django-pam.

Hello, I'm really sorry I haven't replied. Please reopen. I have a stack trace:

ERROR Internal Server Error: /api-token-auth/
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/django/core/handlers/exception.py", line 34, in inner
    response = get_response(request)
  File "/usr/local/lib/python3.6/site-packages/django/core/handlers/base.py", line 126, in _get_response
    response = self.process_exception_by_middleware(e, request)
  File "/usr/local/lib/python3.6/site-packages/django/core/handlers/base.py", line 124, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/usr/local/lib/python3.6/site-packages/django/views/decorators/csrf.py", line 54, in wrapped_view
    return view_func(*args, **kwargs)
  File "/usr/local/lib/python3.6/site-packages/django/views/generic/base.py", line 68, in view
    return self.dispatch(request, *args, **kwargs)
  File "/usr/local/lib/python3.6/site-packages/rest_framework/views.py", line 495, in dispatch
    response = self.handle_exception(exc)
  File "/usr/local/lib/python3.6/site-packages/rest_framework/views.py", line 455, in handle_exception
    self.raise_uncaught_exception(exc)
  File "/usr/local/lib/python3.6/site-packages/rest_framework/views.py", line 492, in dispatch
    response = handler(request, *args, **kwargs)
  File "/usr/local/lib/python3.6/site-packages/rest_framework/authtoken/views.py", line 44, in post
    serializer.is_valid(raise_exception=True)
  File "/usr/local/lib/python3.6/site-packages/rest_framework/serializers.py", line 236, in is_valid
    self._validated_data = self.run_validation(self.initial_data)
  File "/usr/local/lib/python3.6/site-packages/rest_framework/serializers.py", line 437, in run_validation
    value = self.validate(value)
  File "/usr/local/lib/python3.6/site-packages/rest_framework/authtoken/serializers.py", line 21, in validate
    username=username, password=password)
  File "/usr/local/lib/python3.6/site-packages/django/contrib/auth/__init__.py", line 73, in authenticate
    user = backend.authenticate(request, **credentials)
  File "/usr/local/lib/python3.6/site-packages/django_pam/auth/backends.py", line 52, in authenticate
    "The 'request' positonal argument should be either None or an "
AssertionError: The 'request' positonal argument should be either None or an HttpRequest object.
[28/Mar/2019 17:56:07] "POST /api-token-auth/ HTTP/1.1" 500 15677

Python package versions:

• Django==2.1.5
• djangorestframework==3.9.1
• django-pam==1.5.0

My point is, that the request object in the authenticate() method isn't used at all. So there is no need to put such a strict requirement (being of a certain type) on it.
The request argument is filled, but instead of a subclass of django.http.HttpRequest a rest_framework.request.Request is used. There is no problem with that, following duck typing.

from django-pam.

If I take out the strict requirement by removing the assert your code will just fail in a different location. There is a specific pattern that I'm using, defined by Django, for custom backends and it requires a request argument in the first positional location in the authenticate method. The assert just lets you know something needs to be in that position a real request object or None. See https://docs.djangoproject.com/en/2.1/topics/auth/customizing/#writing-an-authentication-backend.

So in other words I cannot remove the request object just because it is never used, since it will break in Django itself somewhere it I do. Just put a None in that position and you're good to go.

from django-pam.

The Django documentation only states that the object in the request argument will be either a HttpRequest object or None. It doesn't tell the custom backend programmer what to do with it.

As PAMBackend.authenticate() doesn't do anything with it, there is no need to enforce a type.

So in other words I cannot remove the request object just because it is never used, since it will break in Django itself somewhere it I do.

It won't - you are not using it.

Just put a None in that position and you're good to go.

I am not putting anything there - its is Django that is calling django-pam:

File "/usr/local/lib/python3.6/site-packages/django/contrib/auth/__init__.py", line 73, in authenticate
    user = backend.authenticate(request, **credentials)

and Django has been totally OK with the rest_framework.request.Request object.

from django-pam.

All my assert does is warn you that the signature is wrong that is being passed to the authenticate method. This tells me there is something wrong with your setup somewhere. Django requires the request argument to be first. If the assert is blowing up then it is not passing the proper arguments. Comment out my assert and I bet you will still have other issues.

I lot of people are using this plugin and nobody has had this issue, so I am NOT taking it out.

The only reason that it would not work properly is that you have rewritten the authenticate function in django/contrib/auth/__init__.py and forgot to pass in a None to the actual backend call. The only other place the backend is called is in the RemoteUser middleware. So one of these two places you must have overridden and could be the cause of the assert to fire.

from django-pam.

Your assert is not a "warning" - it raises an exception.

This is not how Python programming works. It is not the job of a called function to check the type of all of its arguments.
The argument that my code may blow up somewhere else is not relevant - it's not the job of a library to checking the code of its users.

The authenticate functions signature is

def authenticate(request=None, **credentials) -> Union[None, User]

The Django programmers are so kind to tell you in the documentation that the request argument may be None or a HttpRequest object. And the only thing that your function - it is a callback function - has to do is: return either None or a User object. It is not it's job to check whatever is in its arguments. Doing so makes everyones jobs harder - including those of the Django core programmers.

Now: as you can see in the stracktrace above, DRF is calling the authenticate function with a request argument in the first position. Look at the source code of DRF: https://github.com/encode/django-rest-framework/blob/master/rest_framework/authtoken/serializers.py#L20

So everything is fine. The problem that the assert raises is, that the object passed is not of (sub)type django.http.HttpRequest, but of type rest_framework.request.Request. In Python there is the concept of duck typing. Here is good article. What you should check is not the type, but only if the object you are using has the required properties, aka interface.

If you take a look at the rest_framework.request.Request class you can see, that it is a class with an interface that is compatible to django.http.HttpRequest: https://github.com/encode/django-rest-framework/blob/master/rest_framework/request.py

Python duck typing dictates, that if a function gets passed an object that does what it is supposed to do, then the function should simply use it.
As your function isn't using the request object at all, it should not police it at all.

from django-pam.

I really wish you would have mentioned that the DRF Request class did not sub the Django HttpRequest class from the beginning, you would have convinced me a lot sooner. Sorry for the misunderstanding. I will accept your patch soon, however, I'll need to clean up a few things relating to this including some unittests, so don't expect it right away. Hopefully within a week or so. Thanks

from django-pam.

Oh darn - sorry for the missing information!
Thanks for the patient discussion :)

from django-pam.