#21790 closed Bug (fixed)

Don't `except AssertionError` in auth.get_user

Reported by: matklad Owned by: timo
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

Description

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.

Change History (10)

comment:1 Changed 14 months ago by timo

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to timo
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

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 14 months ago by matklad

@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 14 months ago by timo

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 14 months ago by matklad

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 13 months ago by timo

  • Has patch set

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

comment:6 Changed 13 months 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 13 months ago by matklad

Didn't mean to be anonymous.

comment:8 Changed 13 months ago by timo

You are correct, PR updated.

comment:9 Changed 13 months ago by matklad

Looks good now!

comment:10 Changed 13 months ago by Tim Graham <timograham@…>

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

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