Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#15671 closed Bug (fixed)

hasattr in RemoteUserMiddleware hides true errors and exceptions

Reported by: Matt McDonald Owned by:
Component: contrib.auth Version: dev
Severity: Normal Keywords:
Cc: Matt McDonald Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (3)

no-hasattr.diff (2.0 KB ) - added by Matt McDonald 13 years ago.
no-hasattr2.diff (2.0 KB ) - added by Matt McDonald 13 years ago.
no-hasattr3.diff (3.3 KB ) - added by Matt McDonald 13 years ago.

Download all attachments as: .zip

Change History (29)

by Matt McDonald, 13 years ago

Attachment: no-hasattr.diff added

comment:1 by Matt McDonald, 13 years ago

Owner: Matt McDonald removed

comment:2 by Luke Plant, 13 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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.

in reply to:  2 ; comment:3 by anonymous, 13 years ago

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?

comment:4 by Matt McDonald, 13 years ago

Cc: Matt McDonald added

in reply to:  3 comment:5 by Łukasz Rekucki, 13 years ago

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.

by Matt McDonald, 13 years ago

Attachment: no-hasattr2.diff added

comment:6 by Matt McDonald, 13 years ago

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

comment:7 by Luke Plant, 13 years ago

Type: Bug

comment:8 by Luke Plant, 13 years ago

Severity: Normal

comment:9 by Matt McDonald, 13 years ago

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.

by Matt McDonald, 13 years ago

Attachment: no-hasattr3.diff added

comment:10 by Alex Gaynor, 13 years ago

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 :)

comment:11 by Matt McDonald, 13 years ago

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

comment:12 by Gary Wilson, 13 years ago

Easy pickings: unset
Patch needs improvement: unset
Summary: RemoteUserMiddleware hides true errors and exceptionshasattr in RemoteUserMiddleware hides true errors and exceptions
Triage Stage: AcceptedReady for checkin

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.

comment:13 by Matt McDonald, 13 years ago

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.

comment:14 by Chris Beaven, 13 years ago

How about just this?

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

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

comment:15 by Matt McDonald, 13 years ago

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

comment:16 by Chris Beaven, 13 years ago

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

EDIT: specifically, here's a quick test:

Python 2.7.1+ (r271:86832, Apr 11 2011, 18:13:53) 
[GCC 4.5.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> class C(object):
...     @property
...     def x(self):
...         return 1 // 0
... 
>>> c = C()
>>> hasattr(c, 'x')
False
>>> getattr(c, 'x', None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 4, in x
ZeroDivisionError: integer division or modulo by zero
Last edited 13 years ago by Chris Beaven (previous) (diff)

comment:17 by Matt McDonald, 13 years ago

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.

comment:18 by Chris Beaven, 13 years ago

Ok, how about this:

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

comment:19 by Matt McDonald, 13 years ago

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

comment:20 by Alex Gaynor, 13 years ago

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.

comment:21 by Chris Beaven, 13 years ago

metzen: the try is to catch customized __getattribute__ method

in reply to:  20 comment:22 by Matt McDonald, 13 years ago

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.

comment:23 by Luke Plant, 13 years ago

Resolution: fixed
Status: newclosed

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.

comment:24 by anonymous, 13 years ago

UI/UX: unset

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

in reply to:  24 ; comment:25 by dkrieger@…, 12 years ago

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.

Note: See TracTickets for help on using tickets.
Back to Top