Opened 3 years ago

Closed 3 years ago

#21790 closed Bug (fixed)

Don't `except AssertionError` in auth.get_user

Reported by: Aleksey Kladov Owned by: Tim Graham
Component: contrib.auth Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


In django.contrib.auth.get_user
( AssetionError is excepted which causes different behaviour with and without -O interpreter flag.

More previsely, incorrect backend_path in session will cause ImproperlyConfigured exception to be raised with -O flag, while without the flag the AnonymousUser will be returned.

Change History (10)

comment:1 Changed 3 years ago by Tim Graham

Needs documentation: unset
Needs tests: unset
Owner: changed from nobody to Tim Graham
Patch needs improvement: unset
Status: newassigned
Triage Stage: UnreviewedAccepted

I actually addressed this in another PR but I'll pull the fix out from there if the code for that issue gets refactored so as not to need this fix.

comment:2 Changed 3 years ago by Aleksey Kladov

@timo, in your PR you fixed it by changing assert spam to if not spam: raise AssertionError and left except AssertionError as is. This still has a drawback because it silences any potential assertion errors in the code called from try block.

comment:3 Changed 3 years ago by Tim Graham

Under what cases do you see this being a problem? If we remove AssertionError from except, it will be backwards incompatible for any custom authentication backends which raise AssertionError to signal some error.

Would you prefer we raise KeyError or some other exception (which would then be added to the except clause)?

comment:4 Changed 3 years ago by Aleksey Kladov

Under what cases do you see this being a problem?

When assert statement is used in the code to verify invariants.

Suppose I am developing a custom backend. I use assert statements for sanity checks to be able to locate errors quickly. And if I do make a bug and one of my assertions fails, I will not notice it, because no exception will pop up, and I'll spend time to figure out why users are Anonymous, if of course I manage to notice that something is wrong, which also will be difficult in such case.

Removing AssertionError from except will be backwards incompatible, but this behaviour is undocumented and I don't think that a lot of users are relying on it.

Thinking more about this, I find that it's a good idea(modulo backwards compatibility) to move user = backend.get_user(user_id) or AnonymousUser() from try block altogether, because masking any exception from backend(potentially custom one) is not a good idea.

To quote the zen of python

Errors should never pass silently.
Unless explicitly silenced.

comment:5 Changed 3 years ago by Tim Graham

Has patch: set

How does this PR look?

There's an existing test which currently fails when running the tests with python -O django.contrib.auth and passes after the patch.

FAIL: test_changed_backend_settings (django.contrib.auth.tests.test_auth_backends.ChangedBackendSettingsTest)
Traceback (most recent call last):
  File "/home/tim/code/django/django/test/", line 215, in inner
    return test_func(*args, **kwargs)
  File "/home/tim/code/django/django/contrib/auth/tests/", line 480, in test_changed_backend_settings
AssertionError: False is not true

comment:6 Changed 3 years ago by anonymous

Shouldn't you first check that backend_path in settings.AUTHENTICATION_BACKENDS and only after that do backend = load_backend(backend_path)?

comment:7 Changed 3 years ago by Aleksey Kladov

Didn't mean to be anonymous.

comment:8 Changed 3 years ago by Tim Graham

You are correct, PR updated.

comment:9 Changed 3 years ago by Aleksey Kladov

Looks good now!

comment:10 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In e1c8bc8fea1a4eabaa76e4ee3d583c1b4f3796a9:

Fixed #21790 -- Removed reliance on an assert in auth.get_user().

Thanks matklad for the report.

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