#18171 closed Bug (fixed)
TypeErrors pass silently inside of authentication backends `authenticate()`
| Reported by: | 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:
- Check
Exception.messagefor/takes (?:exactly \d+|no) arguments?/and re-raise orlogger.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.messagewould also work)
- 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)
Change History (17)
comment:1 by , 14 years ago
| Cc: | added |
|---|
comment:2 by , 14 years ago
| Triage Stage: | Unreviewed → 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.
by , 14 years ago
| Attachment: | can_authenticate.patch added |
|---|
proposal for adding can_authenticate instead of capturing TypeErrors
comment:3 by , 14 years ago
| Cc: | 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 , 13 years ago
| Owner: | changed from to |
|---|
by , 13 years ago
| Attachment: | type_error_raised.diff added |
|---|
comment:5 by , 13 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 , 13 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 , 13 years ago
| Attachment: | type_error_raised.2.diff added |
|---|
comment:7 by , 13 years ago
| Has patch: | set |
|---|
comment:8 by , 13 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:11 by , 12 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
I do like the approach in the PR linked above (2.7+ only).
comment:12 by , 12 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
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
TypeErrorin my code causing theauthenticate()to fail. If think the second option is the way to go since the first one is too fragile and won't work if aTypeErroris raised in an invalid signature case.