Opened 18 years ago

Closed 11 years ago

#2550 closed New feature (fixed)

Allow a Backend to Globally Fail Authentication

Reported by: umbrae@… 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)

auth-__init__-changed.diff (654 bytes ) - added by umbrae@… 18 years ago.
Patch to implement 'required' functionality
auth-__init__-changed-viewable.diff (917 bytes ) - added by umbrae@… 18 years ago.
A (hopefully) viewable version of the previous diff
patch.diff (3.3 KB ) - added by Ashutosh Dwivedi 13 years ago.
the implementation along with test cases submitted by aashu_dwivedi
patch_corrected.diff (4.3 KB ) - added by Ashutosh Dwivedi 13 years ago.
corrected patch
ticket2550.diff (4.4 KB ) - added by Łukasz Rekucki 13 years ago.
Fixed indentation in the previous patch. Tweaked the docs a bit.
ticket2550b.diff (4.4 KB ) - added by dan.julius@… 13 years ago.
Fixed value of supports_anonymous_user and supports_object_permissions
ticket2550c.diff (4.3 KB ) - added by Daniel Roseman 12 years ago.
Update patch to apply against 1.4
ticket2550d.diff (4.9 KB ) - added by namn 12 years ago.
ticket2550d.diff

Download all attachments as: .zip

Change History (39)

by umbrae@…, 18 years ago

Attachment: auth-__init__-changed.diff added

Patch to implement 'required' functionality

by umbrae@…, 18 years ago

A (hopefully) viewable version of the previous diff

comment:1 by umbrae@…, 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 Simon G. <dev@…>, 17 years ago

Triage Stage: UnreviewedDesign decision needed

comment:3 by Jacob, 16 years ago

Resolution: fixed
Status: newclosed
Triage Stage: Design decision neededAccepted

comment:4 by Gary Wilson, 16 years ago

Resolution: fixed
Status: closedreopened

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 Chris Beaven, 16 years ago

Keywords: easy-pickings added
Triage Stage: AcceptedDesign 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 Malcolm Tredinnick, 14 years ago

Triage Stage: Design decision neededAccepted

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 Ashutosh Dwivedi, 13 years ago

Owner: changed from nobody to Ashutosh Dwivedi
Status: reopenednew

by Ashutosh Dwivedi, 13 years ago

Attachment: patch.diff added

the implementation along with test cases submitted by aashu_dwivedi

comment:8 by Ashutosh Dwivedi, 13 years ago

I am not sure but do i need to update the documentation also ?

comment:9 by Ashutosh Dwivedi, 13 years ago

Triage Stage: AcceptedReady for checkin

comment:10 by Łukasz Rekucki, 13 years ago

Needs documentation: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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).
Last edited 13 years ago by Łukasz Rekucki (previous) (diff)

by Ashutosh Dwivedi, 13 years ago

Attachment: patch_corrected.diff added

corrected patch

comment:11 by Ashutosh Dwivedi, 13 years ago

Triage Stage: AcceptedReady for checkin

Added corrected patch with the required addition to the documentation .

by Łukasz Rekucki, 13 years ago

Attachment: ticket2550.diff added

Fixed indentation in the previous patch. Tweaked the docs a bit.

comment:12 by Łukasz Rekucki, 13 years ago

milestone: 1.4
Needs documentation: unset
Patch needs improvement: unset
Triage Stage: Ready for checkinAccepted

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 Gabriel Hurley, 13 years ago

Component: Contrib appscontrib.auth

comment:14 by Aymeric Augustin, 13 years ago

#15716 was a duplicate.

comment:15 by Łukasz Rekucki, 13 years ago

Type: enhancementNew feature

comment:16 by Łukasz Rekucki, 13 years ago

Severity: normalNormal

comment:17 by Jacob, 13 years ago

Easy pickings: set

comment:18 by anonymous, 13 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 Florian Apolloner, 13 years ago

Cc: florian+django@… added

by dan.julius@…, 13 years ago

Attachment: ticket2550b.diff added

Fixed value of supports_anonymous_user and supports_object_permissions

comment:20 by dan.julius@…, 13 years ago

Patch needs improvement: unset
Triage Stage: AcceptedDesign 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 Carl Meyer, 13 years ago

Triage Stage: Design decision neededAccepted

comment:22 by anonymous, 13 years ago

Resolution: fixed
Status: newclosed

comment:23 by Aymeric Augustin, 13 years ago

Resolution: fixed
Status: closedreopened

Revert apparent spam. If this is really fixed, please provide a short explanation, for instance a commit reference or superseding ticket.

Last edited 13 years ago by Aymeric Augustin (previous) (diff)

comment:24 by Jacob, 12 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:25 by myusuf3, 12 years ago

Resolution: fixed
Status: reopenedclosed

in reply to:  25 comment:26 by Carl Meyer, 12 years ago

Resolution: fixed
Status: closedreopened

Please don't mark tickets fixed without explanation.

by Daniel Roseman, 12 years ago

Attachment: ticket2550c.diff added

Update patch to apply against 1.4

comment:27 by Daniel Roseman, 12 years ago

Triage Stage: AcceptedReady for checkin

comment:28 by Jannis Leidel, 12 years ago

Summary: [patch] Allow a Backend to Globally Fail AuthenticationAllow a Backend to Globally Fail Authentication

comment:29 by Jannis Leidel, 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.

by namn, 12 years ago

Attachment: ticket2550d.diff added

ticket2550d.diff

comment:30 by namn, 12 years ago

Patch needs improvement: unset

Patch revised.

comment:31 by Jannis Leidel <jannis@…>, 11 years ago

Resolution: fixed
Status: reopenedclosed

In 1520748dac95a7f114e4bb2feeee04d46c720494:

Fixed #2550 -- Allow the auth backends to raise the PermissionDenied exception to completely stop the authentication chain. Many thanks to namn, danielr, Dan Julius, Łukasz Rekucki, Aashu Dwivedi and umbrae for working this over the years.

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