Code

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#10901 closed (invalid)

auth.contrib silently catching TypeError

Reported by: anonymous Owned by: nobody
Component: contrib.auth Version: 1.0
Severity: Keywords:
Cc: szabtam@…, humitos@… Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The function authenticate in django/contrib/auth/init.py reads:

31        def authenticate(**credentials):
32          """
33          If the given credentials are valid, return a User object.
34          """
35          for backend in get_backends():
36              try:
37                  user = backend.authenticate(**credentials)
38              except TypeError:
39                  # This backend doesn't accept these credentials as
arguments. Try the next one.
40                  continue
41              if user is None:
42                  continue
43              # Annotate the user object with the path of the backend.
44              user.backend = "%s.%s" % (backend.__module__,
backend.__class__.__name__)
45              return user

As you can see the code catches and silently ignores all TypeError exceptions:
The problems with this approach are:

  • Why not fail as early as possible if one of the authentication

backends configured in settings.py has a wrong signature? If nothing
else at least a warning should be logged IMHO.

  • The bigger problem is that the code silently catches all TypeError

exceptions. If the signature is correct, but the custom backend
authenticator somewhere has a bug and a TypeError is raised as a
result, the exception will be hidden away. TypeError is a common
exception, so I don't think that catching and ignoring it in code that
others will write is a good idea.

Attachments (0)

Change History (4)

comment:1 in reply to: ↑ description ; follow-up: Changed 5 years ago by ramiro

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Replying to anonymous:

  • Why not fail as early as possible if one of the authentication

backends configured in settings.py has a wrong signature? If nothing
else at least a warning should be logged IMHO.

Because, for example, django.contrib.auth.backends.ModelBackend.authenticate() has both user and password parameters and django.contrib.auth.backends.RemoteUserBackend.authenticate() has just one user parameter.

comment:2 in reply to: ↑ 1 Changed 5 years ago by anonymous

Replying to ramiro:

Replying to anonymous:

  • Why not fail as early as possible if one of the authentication

backends configured in settings.py has a wrong signature? If nothing
else at least a warning should be logged IMHO.

Because, for example, django.contrib.auth.backends.ModelBackend.authenticate() has both user and password parameters and django.contrib.auth.backends.RemoteUserBackend.authenticate() has just one user parameter.

Supporting multiple authentication backends is good, but I don't think that the implementations is the best. Please see the related discussion on Django-Users:

http://groups.google.com.au/group/django-users/browse_thread/thread/27076627b7c35e43/7c49a7b61325120b#7c49a7b61325120b

comment:3 Changed 5 years ago by mtredinnick

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

This isn't a bug for the reasons explained here and in the original thread on django-users. We are using TypeError in the way it is intended in Python. That it happens to also disguise programmer errors is not really our problem, since people shouldn't be relying on it to that (programmer errors should be caught earlier). This is an expected exceptional condition that we are handling. We are handling in a deliberate fashion by realising that it's not a problem for our execution path.

And if none of that convinced you, I'm sorry, but changing it would also be backwards incompatible and even though this is a contrib app and we can change things that are geuine bugs, this isn't a bug and there are a lot of custom authentication backends in the wild.

comment:4 Changed 3 years ago by humitos

  • Cc humitos@… added

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.