Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#18171 closed Bug (fixed)

TypeErrors pass silently inside of authentication backends `authenticate()`

Reported by: David Eyk <deyk@…> Owned by: Renato Oliveira
Component: contrib.auth Version: 1.4
Severity: Normal Keywords:
Cc: charette.s@…, jeffhui Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

TypeErrors pass silently when raised inside of an authentication backend's authenticate method. This can make debugging a backend tricky.

A simple example:

class MyBackend(object):
    def authenticate(self, username, password):
        for item in None:  # Raises a TypeError, which passes silently!
            print "You'll never get here!"

Because django.auth.authenticate uses TypeErrors to check whether the backend has been called with the correct signature, there may not be an easy solution for this, but some options would include:

  1. Check Exception.message for /takes (?:exactly \d+|no) arguments?/ and re-raise or logger.exception() if it doesn't match. This would significantly narrow down the cases where the wrong exception would silently pass. (A simpler, quicker check of "argument" in e.message would also work)
  1. Document this behavior, recommending that backends should handle their own TypeErrors in https://docs.djangoproject.com/en/1.4/topics/auth/#writing-an-authentication-backend

Attachments (4)

can_authenticate.patch (2.3 KB ) - added by jeffhui 12 years ago.
proposal for adding can_authenticate instead of capturing TypeErrors
type_error_raised.diff (670 bytes ) - added by Renato Oliveira 12 years ago.
type_error_raised.2.diff (718 bytes ) - added by Renato Oliveira 12 years ago.
type_error_raised.3.diff (861 bytes ) - added by Renato Oliveira 12 years ago.
Just correcting a mistake

Download all attachments as: .zip

Change History (17)

comment:1 by Simon Charette, 12 years ago

Cc: charette.s@… added

I've got bitten by that recently. The django-openid-auth project changed a method signature in a recent release and it was raising a TypeError in my code causing the authenticate() to fail. If think the second option is the way to go since the first one is too fragile and won't work if a TypeError is raised in an invalid signature case.

comment:2 by Aymeric Augustin, 12 years ago

Triage Stage: UnreviewedAccepted

Me too — https://github.com/aaugustin/django-urlauth/blob/master/urlauth/backends.py#L66 :(

To be honest, neither solution thrills me, but I'm ready to give feedback on patches improving the situation.

by jeffhui, 12 years ago

Attachment: can_authenticate.patch added

proposal for adding can_authenticate instead of capturing TypeErrors

comment:3 by jeffhui, 12 years ago

Cc: jeffhui added

Perhaps adding a can_authenticate method would be better? can_authenticate should be required at some point in time for backends.

comment:4 by Renato Oliveira, 12 years ago

Owner: changed from nobody to Renato Oliveira

by Renato Oliveira, 12 years ago

Attachment: type_error_raised.diff added

comment:5 by Renato Oliveira, 12 years ago

I've just raised the exception showing its message, I don't know if it's right, waiting for a feedback.

comment:6 by Renato Oliveira, 12 years ago

Just added a line, which checks if the message if the word 'argument' is in, to keep checking the user's backends. It isn't the fancy solution but fix the error.

by Renato Oliveira, 12 years ago

Attachment: type_error_raised.2.diff added

by Renato Oliveira, 12 years ago

Attachment: type_error_raised.3.diff added

Just correcting a mistake

comment:7 by Renato Oliveira, 12 years ago

Has patch: set

comment:8 by Ruy Asan, 11 years ago

I too have been bitten by this (#19337).

Is there any particular reason backends can't signal "I cannot handle this signature" by returning None (as is already the case actually)?

Or, if we want to be fastidious about it, return some aptly named constant like contrib.auth.CannotHandle. Anything explicit instead of incidental will do really.

Existing backends would have to be updated. A planned deprecation could be introduced by initially accepting TypeError as before, but also issuing a deprecation nag/warning, and subsequently, re-raising TypeErrors from the same spot but with an added explanation that it's probably just your backend that needs to be updated.

Checking for a particular subtype of TypeError (i.e. argument signature mismatch specifically) simply narrows the number of real backend errors that can be masked, but IMO, it's still a reasonably common error...

comment:9 by anonymous, 11 years ago

Got same problem. +1 for can_authenticate()

comment:11 by Claude Paroz, 11 years ago

Triage Stage: AcceptedReady for checkin

I do like the approach in the PR linked above (2.7+ only).

comment:12 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: newclosed

In b89c2a5d9eb70ca36629ef657c98e3371e9a5c4f:

Fixed #18171 -- Checked signature of authenticate() to avoid supressing TypeErrors.

The current auth backend code catches TypeError to detect backends that
do not support specified argumetnts. As a result, any TypeErrors raised
within the actual backend code are silenced.

In Python 2.7+ and 3.2+ this can be avoided by using inspect.getcallargs().
With this method, we can test whether arguments match the signature without
actually calling the function.

Thanks David Eyk for the report.

comment:13 by David Eyk, 11 years ago

Thanks to everybody who worked on this. I learned a new trick!

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