hasattr in RemoteUserMiddleware hides true errors and exceptions

[Matt McDonald]

The RemoteUserMiddleware hides true errors and exceptions that may occur when fetching Session and User data from the db backend. This occurs because the middleware first performs a test to ensure that contrib.auth.middleware.AuthenticationMiddleware has been installed by calling:

if not hasattr(request, 'user'):
    raise ImproperlyConfigured(...)

However, hasattr dangerously catches all exceptions that occur during the call, which hides any of the multitude of problems that could occur when the LazyUser attribute attempts to fetch the real User instance from the database.

I've proposed a patch which instead uses a try/except on AttributeError to perform the same logic, while letting other exceptions correctly bubble up.

[Luke Plant]

Accepted. The test needs a slight improvement - the line middleware.LazyUser = curr_lazy_user needs to be in a finally clause to ensure it runs.

[anonymous]

Replying to lukeplant:

Accepted. The test needs a slight improvement - the line middleware.LazyUser = curr_lazy_user needs to be in a finally clause to ensure it runs.

Is that really necessary? The assertRaises call will catch the exception that I'm attempting to test with (TypeError), and the code that follows it will always run, no?

[Łukasz Rekucki]

Replying to anonymous:

Replying to lukeplant:

Accepted. The test needs a slight improvement - the line middleware.LazyUser = curr_lazy_user needs to be in a finally clause to ensure it runs.

Is that really necessary? The assertRaises call will catch the exception that I'm attempting to test with (TypeError), and the code that follows it will always run, no?

It won't run, if the function doesn't throw TypeError. Then assertRaises will raise AssertionError and the cleanup code won't run.

[Matt McDonald]

Hmm, on second thought, I suppose it couldn't hurt, just to ensure the monkey patching is reset in all cases. New patch attached.

[Matt McDonald]

I've found an issue with the current patch. In order to be fully robust, this should also allow AttributeErrors raised during the process of fetching the User object to bubble up. New patch attached with some additional tests.

[Alex Gaynor]

I'd prefer this did hasattr(request.__class__, "user"), which seems to fit the constraint of not evaluating the property, as well as the property of not being unnecessarily obscure :)

[Matt McDonald]

But this would evaluate the descriptor, and fall into the same original trap.

[Gary Wilson]

FWIW, this "broken" hasattr behavior seems to have recently been fixed for Python 3.2, see ​http://bugs.python.org/issue9666 and ​http://docs.python.org/py3k/whatsnew/3.2.html. Also, hasattr calls getattr anyway so we wouldn't be doing any extra work if we were to just forgo the hasattr call and just try the attribute lookup (we later use the request.user lookup anyhow, so the point is moot).

Patch 3 wouldn't work if someone were doing something funky with request and __getattribute__.

I'm happy with putting in a workaround instead of waiting for Python 3.2 support, and would favor patch 2 here due to it using a more Pythonic lookup.

[Matt McDonald]

Would a solution using dir() work better with a customized __getattribute__?

if 'user' not in dir(request):
  raise ImproperlyConfigured

Alternatively, could something be done with inspecting the stack trace to find where the exception originated?

The lack of patch2 to correctly raise AttributeErrors beneath the request.user lookup has actually bitten me several times in my current project.

[Chris Beaven]

How about just this?

    if getattr(request, 'user', None) is None:

(plus a comment on why we're doing this rather than just hasattr)

[Matt McDonald]

getattr would also conceal errors in the same manner as hasattr.

[Chris Beaven]

Are you sure? I was under the impression that hasattr is what was suppressing the errors.

[Matt McDonald]

Ah sorry, it won't conceal all errors like hasattr, but it will conceal AttributeErrors, much like the solution in patch 1 and 2.

>>> class C(object):
...   @property
...   def x(self):
...     raise AttributeError
... 
>>> c = C()
>>> getattr(c, 'x')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 4, in x
AttributeError
>>> getattr(c, 'x', None) is None
True

This would actually be correct behavior for getattr, but it would be better if the solution used here was robust enough to allow those errors to bubble up. I've run into this while working on a custom db backend, and it's quite painful to keep seeing your true coding errors concealed behind a message stating that you need to correct your MIDDLEWARE_CLASSES.

[Chris Beaven]

Ok, how about this:

try: 
  request.user 
except AttributeError: 
  if 'user' in dir(request):
    raise
  raise ImproperlyConfigured(
    ...

[Matt McDonald]

The try/except is a bit redundant in that case; you could simply just perform the 'user' in dir(request) test alone.

However, this pretty much the same solution I proposed in ​http://code.djangoproject.com/ticket/15671#comment:13, where I was also unsure about how it would behave in situations with a customized __getattribute__ method, as pointed out in ​http://code.djangoproject.com/ticket/15671#comment:12

[Alex Gaynor]

Please no dir in runtime code :(, just use getattr, if things are letting AttributeError escape to mean something other than "this attribute doesn't exist", write more unittests.

[Chris Beaven]

metzen: the try is to catch customized __getattribute__ method

[Matt McDonald]

Replying to Alex:

Please no dir in runtime code :(, just use getattr, if things are letting AttributeError escape to mean something other than "this attribute doesn't exist", write more unittests.

There is already a test in no-hasattr3.diff which demonstrates this case.

[Luke Plant]

The implementation of the 'user' attribute added by the auth middleware has recently completed changed. There is no longer a LazyUser object.

The changes and tests in most recent patches assume the old implementation, which shows that the patch has the wrong approach - it shouldn't be depending on an implementation detail of another piece of code (especially the tests - I guess the middleware changes are more reasonably since it lives in the same module as the LazyUser class).

The only thing we can do is use 'getattr', and if other things are letting AttributeError bubble up there is nothing we can sensibly do. However, in the current situation, there is no reason for even that, since I'm pretty sure the new implementation fixes the bug here i.e. doing `hasattr(request, 'user') will not mask any exceptions from the DB layer, because it doesn't cause the DB layer to be invoked.

Please re-open if the bug is not fixed *and* there is something we can do to fix it that does not depend on the implementation of the auth middleware.

[anonymous]

The new implementation does seem to avoid the issues with hasattr, making my patch unnecessary. Thanks!

[dkrieger@…]

May I ask, in which release did this fix go out? I am encountering the conditions described ("ImproperlyConfigured" error complaining that AuthenticationMiddleware needs to be in MIDDLEWARE_CLASSES before RemoteUserMiddleware, even though that is already the case) in Django 1.2.5.

[dkrieger@…]

Replying to dkrieger@…:
NM, it's 1.4 (​https://github.com/django/django/blob/stable/1.3.x/django/contrib/auth/middleware.py vs ​https://github.com/django/django/blob/stable/1.4.x/django/contrib/auth/middleware.py )