#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)
Change History (29)
by , 14 years ago
Attachment: | no-hasattr.diff added |
---|
comment:1 by , 14 years ago
Owner: | removed |
---|
follow-up: 3 comment:2 by , 14 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
follow-up: 5 comment:3 by , 14 years ago
Replying to lukeplant:
Accepted. The test needs a slight improvement - the line
middleware.LazyUser = curr_lazy_user
needs to be in afinally
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 , 14 years ago
Cc: | added |
---|
comment:5 by , 14 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 afinally
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 , 14 years ago
Attachment: | no-hasattr2.diff added |
---|
comment:6 by , 14 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 , 14 years ago
Type: | → Bug |
---|
comment:8 by , 14 years ago
Severity: | → Normal |
---|
comment:9 by , 14 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 , 14 years ago
Attachment: | no-hasattr3.diff added |
---|
comment:10 by , 14 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 , 14 years ago
But this would evaluate the descriptor, and fall into the same original trap.
comment:12 by , 14 years ago
Easy pickings: | unset |
---|---|
Patch needs improvement: | unset |
Summary: | RemoteUserMiddleware hides true errors and exceptions → hasattr in RemoteUserMiddleware hides true errors and exceptions |
Triage Stage: | Accepted → 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 by , 14 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 , 14 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:16 by , 14 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
comment:17 by , 14 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 , 14 years ago
Ok, how about this:
try: request.user except AttributeError: if 'user' in dir(request): raise raise ImproperlyConfigured( ...
comment:19 by , 14 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
follow-up: 22 comment:20 by , 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:22 by , 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 , 13 years ago
Resolution: | → fixed |
---|---|
Status: | new → 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.
follow-up: 25 comment:24 by , 13 years ago
UI/UX: | unset |
---|
The new implementation does seem to avoid the issues with hasattr, making my patch unnecessary. Thanks!
follow-up: 26 comment:25 by , 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.
Accepted. The test needs a slight improvement - the line
middleware.LazyUser = curr_lazy_user
needs to be in afinally
clause to ensure it runs.