Don't `except AssertionError` in auth.get_user

[Aleksey Kladov]

In django.contrib.auth.get_user
(​https://github.com/django/django/blob/master/django/contrib/auth/__init__.py#L151) 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.

[Tim Graham]

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.

[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.

[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)?

[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.

[Tim Graham]

How does this ​PR look?

There's an existing test which currently fails when running the tests with python -O runtests.py 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/utils.py", line 215, in inner
    return test_func(*args, **kwargs)
  File "/home/tim/code/django/django/contrib/auth/tests/test_auth_backends.py", line 480, in test_changed_backend_settings
    self.assertTrue(user.is_anonymous())
AssertionError: False is not true

[anonymous]

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

[Aleksey Kladov]

Didn't mean to be anonymous.

[Tim Graham]

You are correct, PR updated.

[Aleksey Kladov]

Looks good now!

[Tim Graham <timograham@…>]

In e1c8bc8fea1a4eabaa76e4ee3d583c1b4f3796a9:

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

Thanks matklad for the report.