Code

Opened 3 years ago

Closed 3 years ago

Last modified 22 months ago

#15671 closed Bug (fixed)

hasattr in RemoteUserMiddleware hides true errors and exceptions

Reported by: metzen Owned by:
Component: contrib.auth Version: master
Severity: Normal Keywords:
Cc: metzen 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 metzen 3 years ago.
no-hasattr2.diff (2.0 KB) - added by metzen 3 years ago.
no-hasattr3.diff (3.3 KB) - added by metzen 3 years ago.

Download all attachments as: .zip

Change History (29)

Changed 3 years ago by metzen

comment:1 Changed 3 years ago by metzen

  • Needs documentation unset
  • Needs tests unset
  • Owner metzen deleted
  • Patch needs improvement unset

comment:2 follow-up: Changed 3 years ago by lukeplant

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 3 years ago by 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?

comment:4 Changed 3 years ago by metzen

  • Cc metzen added

comment:5 in reply to: ↑ 3 Changed 3 years ago by lrekucki

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.

Changed 3 years ago by metzen

comment:6 Changed 3 years ago by metzen

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 Changed 3 years ago by lukeplant

  • Type set to Bug

comment:8 Changed 3 years ago by lukeplant

  • Severity set to Normal

comment:9 Changed 3 years ago by metzen

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.

Changed 3 years ago by metzen

comment:10 Changed 3 years ago by Alex

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 Changed 3 years ago by metzen

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

comment:12 Changed 3 years ago by gwilson

  • Easy pickings unset
  • Patch needs improvement unset
  • Summary changed from RemoteUserMiddleware hides true errors and exceptions to hasattr in RemoteUserMiddleware hides true errors and exceptions
  • Triage Stage changed from Accepted to Ready 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 Changed 3 years ago by metzen

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 Changed 3 years ago by SmileyChris

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 Changed 3 years ago by metzen

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

comment:16 Changed 3 years ago by SmileyChris

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 3 years ago by SmileyChris (previous) (diff)

comment:17 Changed 3 years ago by metzen

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 Changed 3 years ago by SmileyChris

Ok, how about this:

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

comment:19 Changed 3 years ago by metzen

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 follow-up: Changed 3 years ago by 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.

comment:21 Changed 3 years ago by SmileyChris

metzen: the try is to catch customized __getattribute__ method

comment:22 in reply to: ↑ 20 Changed 3 years ago by metzen

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 Changed 3 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from new to closed

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 follow-up: Changed 3 years ago by anonymous

  • UI/UX unset

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

comment:25 in reply to: ↑ 24 ; follow-up: Changed 22 months ago by 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.