Fix AttributeError hiding on request authenticators (#5600)

* Update assertion style in user logout test

* Apply middlewares to django request object

* Fix test for request auth hiding AttributeErrors

* Re-raise/wrap auth attribute errors

* Fix test for py2k

* Add docs for WrappedAttributeError

Author: Ryan P Kilby

docs/api-guide/authentication.md

@@ -291,6 +291,12 @@ You *may* also override the `.authenticate_header(self, request)` method.  If im
 
 If the `.authenticate_header()` method is not overridden, the authentication scheme will return `HTTP 403 Forbidden` responses when an unauthenticated request is denied access.
 
+---
+
+**Note:** When your custom authenticator is invoked by the request object's `.user` or `.auth` properties, you may see an `AttributeError` re-raised as a `WrappedAttributeError`. This is necessary to prevent the original exception from being suppressed by the outer property access. Python will not recognize that the `AttributeError` orginates from your custom authenticator and will instead assume that the request object does not have a `.user` or `.auth` property. These errors should be fixed or otherwise handled by your authenticator.
+
+---
+
 ## Example
 
 The following example will authenticate any incoming request as the user given by the username in a custom request header named 'X_USERNAME'.

docs/api-guide/requests.md

@@ -90,6 +90,10 @@ You won't typically need to access this property.
 
 ---
 
+**Note:** You may see a `WrappedAttributeError` raised when calling the `.user` or `.auth` properties. These errors originate from an authenticator as a standard `AttributeError`, however it's necessary that they be re-raised as a different exception type in order to prevent them from being suppressed by the outer property access. Python will not recognize that the `AttributeError` orginates from the authenticator and will instaed assume that the request object does not have a `.user` or `.auth` property. The authenticator will need to be fixed.
+
+---
+
 # Browser enhancements
 
 REST framework supports a few browser enhancements such as browser-based `PUT`, `PATCH` and `DELETE` forms.

rest_framework/request.py

@@ -10,6 +10,9 @@
 """
 from __future__ import unicode_literals
 
+import sys
+from contextlib import contextmanager
+
 from django.conf import settings
 from django.http import HttpRequest, QueryDict
 from django.http.multipartparser import parse_header
@@ -59,6 +62,24 @@ def __exit__(self, *args, **kwarg):
         self.view.action = self.action
 
 
+class WrappedAttributeError(Exception):
+    pass
+
+
+@contextmanager
+def wrap_attributeerrors():
+    """
+    Used to re-raise AttributeErrors caught during authentication, preventing
+    these errors from otherwise being handled by the attribute access protocol.
+    """
+    try:
+        yield
+    except AttributeError:
+        info = sys.exc_info()
+        exc = WrappedAttributeError(str(info[1]))
+        six.reraise(type(exc), exc, info[2])
+
+
 class Empty(object):
     """
     Placeholder for unset attributes.
@@ -197,7 +218,8 @@ def user(self):
         by the authentication classes provided to the request.
         """
         if not hasattr(self, '_user'):
-            self._authenticate()
+            with wrap_attributeerrors():
+                self._authenticate()
         return self._user
 
     @user.setter
@@ -220,7 +242,8 @@ def auth(self):
         request, such as an authentication token.
         """
         if not hasattr(self, '_auth'):
-            self._authenticate()
+            with wrap_attributeerrors():
+                self._authenticate()
         return self._auth
 
     @auth.setter
@@ -239,7 +262,8 @@ def successful_authenticator(self):
         to authenticate the request, or `None`.
         """
         if not hasattr(self, '_authenticator'):
-            self._authenticate()
+            with wrap_attributeerrors():
+                self._authenticate()
         return self._authenticator
 
     def _load_data_and_files(self):
@@ -322,7 +346,7 @@ def _parse(self):
 
         try:
             parsed = parser.parse(stream, media_type, self.parser_context)
-        except:
+        except Exception:
             # If we get an exception during parsing, fill in empty data and
             # re-raise.  Ensures we don't simply repeat the error when
             # attempting to render the browsable renderer response, or when

tests/test_request.py

@@ -6,8 +6,10 @@
 import os.path
 import tempfile
 
+import pytest
 from django.conf.urls import url
 from django.contrib.auth import authenticate, login, logout
+from django.contrib.auth.middleware import AuthenticationMiddleware
 from django.contrib.auth.models import User
 from django.contrib.sessions.middleware import SessionMiddleware
 from django.core.files.uploadedfile import SimpleUploadedFile
@@ -17,7 +19,7 @@
 from rest_framework import status
 from rest_framework.authentication import SessionAuthentication
 from rest_framework.parsers import BaseParser, FormParser, MultiPartParser
-from rest_framework.request import Request
+from rest_framework.request import Request, WrappedAttributeError
 from rest_framework.response import Response
 from rest_framework.test import APIClient, APIRequestFactory
 from rest_framework.views import APIView
@@ -197,7 +199,8 @@ def setUp(self):
         # available to login and logout functions
         self.wrapped_request = factory.get('/')
         self.request = Request(self.wrapped_request)
-        SessionMiddleware().process_request(self.request)
+        SessionMiddleware().process_request(self.wrapped_request)
+        AuthenticationMiddleware().process_request(self.wrapped_request)
 
         User.objects.create_user('ringo', '[email protected]', 'yellow')
         self.user = authenticate(username='ringo', password='yellow')
@@ -212,9 +215,9 @@ def test_user_can_login(self):
 
     def test_user_can_logout(self):
         self.request.user = self.user
-        self.assertFalse(self.request.user.is_anonymous)
+        assert not self.request.user.is_anonymous
         logout(self.request)
-        self.assertTrue(self.request.user.is_anonymous)
+        assert self.request.user.is_anonymous
 
     def test_logged_in_user_is_set_on_wrapped_request(self):
         login(self.request, self.user)
@@ -227,22 +230,27 @@ def test_calling_user_fails_when_attribute_error_is_raised(self):
         """
         class AuthRaisesAttributeError(object):
             def authenticate(self, request):
-                import rest_framework
-                rest_framework.MISSPELLED_NAME_THAT_DOESNT_EXIST
+                self.MISSPELLED_NAME_THAT_DOESNT_EXIST
 
-        self.request = Request(factory.get('/'), authenticators=(AuthRaisesAttributeError(),))
-        SessionMiddleware().process_request(self.request)
+        request = Request(self.wrapped_request, authenticators=(AuthRaisesAttributeError(),))
 
-        login(self.request, self.user)
-        try:
-            self.request.user
-        except AttributeError as error:
-            assert str(error) in (
-                "'module' object has no attribute 'MISSPELLED_NAME_THAT_DOESNT_EXIST'",  # Python < 3.5
-                "module 'rest_framework' has no attribute 'MISSPELLED_NAME_THAT_DOESNT_EXIST'",  # Python >= 3.5
-            )
-        else:
-            assert False, 'AttributeError not raised'
+        # The middleware processes the underlying Django request, sets anonymous user
+        assert self.wrapped_request.user.is_anonymous
+
+        # The DRF request object does not have a user and should run authenticators
+        expected = r"no attribute 'MISSPELLED_NAME_THAT_DOESNT_EXIST'"
+        with pytest.raises(WrappedAttributeError, match=expected):
+            request.user
+
+        # python 2 hasattr fails for *any* exception, not just AttributeError
+        if six.PY2:
+            return
+
+        with pytest.raises(WrappedAttributeError, match=expected):
+            hasattr(request, 'user')
+
+        with pytest.raises(WrappedAttributeError, match=expected):
+            login(request, self.user)
 
 
 class TestAuthSetter(TestCase):