Opened 3 years ago

Closed 23 months ago

Last modified 23 months ago

#18171 closed Bug (fixed)

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

Reported by: David Eyk <deyk@…> Owned by: renatooliveira
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 3 years ago.
proposal for adding can_authenticate instead of capturing TypeErrors
type_error_raised.diff (670 bytes) - added by renatooliveira 3 years ago.
type_error_raised.2.diff (718 bytes) - added by renatooliveira 3 years ago.
type_error_raised.3.diff (861 bytes) - added by renatooliveira 3 years ago.
Just correcting a mistake

Download all attachments as: .zip

Change History (17)

comment:1 Changed 3 years ago by charettes

  • Cc charette.s@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 3 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

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.

Changed 3 years ago by jeffhui

proposal for adding can_authenticate instead of capturing TypeErrors

comment:3 Changed 3 years ago by jeffhui

  • 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 Changed 3 years ago by renatooliveira

  • Owner changed from nobody to renatooliveira

Changed 3 years ago by renatooliveira

comment:5 Changed 3 years ago by renatooliveira

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

comment:6 Changed 3 years ago by renatooliveira

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.

Changed 3 years ago by renatooliveira

Changed 3 years ago by renatooliveira

Just correcting a mistake

comment:7 Changed 3 years ago by renatooliveira

  • Has patch set

comment:8 Changed 3 years ago by rubyruy

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 Changed 2 years ago by anonymous

Got same problem. +1 for can_authenticate()

comment:11 Changed 23 months ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

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

comment:12 Changed 23 months ago by Tim Graham <timograham@…>

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

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 Changed 23 months ago by eykd

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

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