Opened 18 years ago
Closed 12 years ago
#2550 closed New feature (fixed)
Allow a Backend to Globally Fail Authentication
Reported by: | Owned by: | Ashutosh Dwivedi | |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | authentication backends easy-pickings |
Cc: | florian+django@… | 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
Allows a backend to fail all authentication - I.E. if a user exists in a backend but the password is wrong, it's very possible that the backend might just want to kick the user out instead of letting it go through the rest of the authentication backends and possibly succeeding.
Kind of like PAM's 'required' option.
Attachments (8)
Change History (39)
by , 18 years ago
Attachment: | auth-__init__-changed.diff added |
---|
by , 18 years ago
Attachment: | auth-__init__-changed-viewable.diff added |
---|
A (hopefully) viewable version of the previous diff
comment:1 by , 18 years ago
Well, I've obviously shown that I'm pretty incompetent when it comes to using tickets. (Sorry guys, I'm a newbie.)
Here's the diff:
Index: trunk/django/contrib/auth/__init__.py =================================================================== --- trunk/django/contrib/auth/__init__.py (revision 3596) +++ trunk/django/contrib/auth/__init__.py (working copy) @@ -1,4 +1,4 @@ -from django.core.exceptions import ImproperlyConfigured +from django.core.exceptions import ImproperlyConfigured, PermissionDenied SESSION_KEY = '_auth_user_id' BACKEND_SESSION_KEY = '_auth_user_backend' @@ -35,6 +35,9 @@ except TypeError: # This backend doesn't accept these credentials as arguments. Try the next one. continue + except PermissionDenied: + # This backend says to stop in our tracks - this user should not be allowed in at all. + return None if user is None: continue # Annotate the user object with the path of the backend.
comment:2 by , 18 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:3 by , 17 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Triage Stage: | Design decision needed → Accepted |
comment:4 by , 17 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Jacob, did you mean to close this or mark it accepted?
I think this we be more flexible if it were part of the AUTHENTICATION_BACKENDS settings instead of hardcoding it into a backend. Authentication backend re-use would go out the window if people started raising PermissionDenied in their backends.
comment:5 by , 17 years ago
Keywords: | easy-pickings added |
---|---|
Triage Stage: | Accepted → Design decision needed |
I agree with gwilson, it's probably better to have a new setting to control it (it seems more of a project-level choice).
I'll push back to design decision anyway
comment:6 by , 14 years ago
Triage Stage: | Design decision needed → Accepted |
---|
Any auth backend should be able to raise "stop here" (return "no", not "don't care"). What special value to return is up to the implementor. It's purely part of the auth backend, not a setting.
comment:7 by , 14 years ago
Owner: | changed from | to
---|---|
Status: | reopened → new |
by , 14 years ago
Attachment: | patch.diff added |
---|
the implementation along with test cases submitted by aashu_dwivedi
comment:9 by , 14 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:10 by , 14 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
Triage Stage: | Ready for checkin → Accepted |
Yes, otherwise people won't even know the feature is available.
Some comments to the patch:
- Your tests fail (did you run them?):
Traceback (most recent call last): File "/home/lrekucki/django/django-gh/django/contrib/auth/tests/auth_backends.py", line 388, in test_authenticates self.assertEqual(authenticate(username='test',password='test'),self.user1) NameError: global name 'authenticate' is not defined
- Some of your code (like "except PermissionDenied" in
django/contrib/__init__.py
) is indented with tabs. Mixin spaces and tabs is bad. Django prefers 4 spaces as indentation. - You should also format your code according to PEP8 (i.e. whitespace around operators, whitespace after comma in argument list).
comment:11 by , 14 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Added corrected patch with the required addition to the documentation .
by , 14 years ago
Attachment: | ticket2550.diff added |
---|
Fixed indentation in the previous patch. Tweaked the docs a bit.
comment:12 by , 14 years ago
milestone: | → 1.4 |
---|---|
Needs documentation: | unset |
Patch needs improvement: | unset |
Triage Stage: | Ready for checkin → Accepted |
On a bureaucratic note: you shouldn't mark your own patches as "Ready for checkin". Instead update the appropriate flags and someone else will review the patch (also see the contributing howto).
Thanks for your work on this, aashu_dwivedi.
comment:13 by , 14 years ago
Component: | Contrib apps → contrib.auth |
---|
comment:15 by , 14 years ago
Type: | enhancement → New feature |
---|
comment:16 by , 14 years ago
Severity: | normal → Normal |
---|
comment:17 by , 14 years ago
Easy pickings: | set |
---|
comment:18 by , 14 years ago
Patch needs improvement: | set |
---|
PermissionDeniedBackend
should set supports_anonymous_user
and supports_object_permissions
to True
since that will be the default soon (and the only option). And if we are already on it: Shouldn't the permission part of the backends support the same mechanism?
comment:19 by , 14 years ago
Cc: | added |
---|
by , 13 years ago
Attachment: | ticket2550b.diff added |
---|
Fixed value of supports_anonymous_user and supports_object_permissions
comment:20 by , 13 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Design decision needed |
UI/UX: | unset |
Modified PermissionDeniedBackend class:
- Set supports_anonymous_user and supports_object_permissions to True
- Added supports_inactive_user = True (which will be required in v1.5)
Implementing similar functionality for permissions will require design change regarding get_all_permissions.
Since each backend provides a list of permissions it allows, the list would be inconsistent if a backend raised the PermissionDenied exception on a permission already in the list.
comment:21 by , 13 years ago
Triage Stage: | Design decision needed → Accepted |
---|
comment:22 by , 13 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:23 by , 13 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Revert apparent spam. If this is really fixed, please provide a short explanation, for instance a commit reference or superseding ticket.
follow-up: 26 comment:25 by , 13 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
comment:26 by , 13 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Please don't mark tickets fixed without explanation.
comment:27 by , 13 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:28 by , 12 years ago
Summary: | [patch] Allow a Backend to Globally Fail Authentication → Allow a Backend to Globally Fail Authentication |
---|
comment:29 by , 12 years ago
Patch needs improvement: | set |
---|
Please update the docs release notes about the changes and use override_settings
in the tests for easier settings switching per test.
comment:31 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Patch to implement 'required' functionality