Opened 10 years ago

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

Download all attachments as: .zip

Change History (39)

Changed 10 years ago by umbrae@…

Attachment: auth-__init__-changed.diff added

Patch to implement 'required' functionality

Changed 10 years ago by umbrae@…

A (hopefully) viewable version of the previous diff

comment:1 Changed 10 years ago by umbrae@…

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 Changed 10 years ago by Simon G. <dev@…>

Triage Stage: UnreviewedDesign decision needed

comment:3 Changed 9 years ago by Jacob

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

comment:4 Changed 9 years ago by Gary Wilson

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 Changed 9 years ago by Chris Beaven

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 Changed 6 years ago by Malcolm Tredinnick

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 Changed 6 years ago by Ashutosh Dwivedi

Owner: changed from nobody to Ashutosh Dwivedi
Status: reopenednew

Changed 6 years ago by Ashutosh Dwivedi

Attachment: patch.diff added

the implementation along with test cases submitted by aashu_dwivedi

comment:8 Changed 6 years ago by Ashutosh Dwivedi

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

comment:9 Changed 6 years ago by Ashutosh Dwivedi

Triage Stage: AcceptedReady for checkin

comment:10 Changed 6 years ago by Łukasz Rekucki

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 6 years ago by Łukasz Rekucki (previous) (diff)

Changed 6 years ago by Ashutosh Dwivedi

Attachment: patch_corrected.diff added

corrected patch

comment:11 Changed 6 years ago by Ashutosh Dwivedi

Triage Stage: AcceptedReady for checkin

Added corrected patch with the required addition to the documentation .

Changed 6 years ago by Łukasz Rekucki

Attachment: ticket2550.diff added

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

comment:12 Changed 6 years ago by Łukasz Rekucki

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 Changed 6 years ago by Gabriel Hurley

Component: Contrib appscontrib.auth

comment:14 Changed 6 years ago by Aymeric Augustin

#15716 was a duplicate.

comment:15 Changed 6 years ago by Łukasz Rekucki

Type: enhancementNew feature

comment:16 Changed 6 years ago by Łukasz Rekucki

Severity: normalNormal

comment:17 Changed 6 years ago by Jacob

Easy pickings: set

comment:18 Changed 6 years ago by anonymous

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 Changed 6 years ago by Florian Apolloner

Cc: florian+django@… added

Changed 5 years ago by dan.julius@…

Attachment: ticket2550b.diff added

Fixed value of supports_anonymous_user and supports_object_permissions

comment:20 Changed 5 years ago by dan.julius@…

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 Changed 5 years ago by Carl Meyer

Triage Stage: Design decision neededAccepted

comment:22 Changed 5 years ago by anonymous

Resolution: fixed
Status: newclosed

comment:23 Changed 5 years ago by Aymeric Augustin

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 5 years ago by Aymeric Augustin (previous) (diff)

comment:24 Changed 5 years ago by Jacob

milestone: 1.4

Milestone 1.4 deleted

comment:25 Changed 5 years ago by myusuf3

Resolution: fixed
Status: reopenedclosed

comment:26 in reply to:  25 Changed 5 years ago by Carl Meyer

Resolution: fixed
Status: closedreopened

Please don't mark tickets fixed without explanation.

Changed 5 years ago by Daniel Roseman

Attachment: ticket2550c.diff added

Update patch to apply against 1.4

comment:27 Changed 5 years ago by Daniel Roseman

Triage Stage: AcceptedReady for checkin

comment:28 Changed 4 years ago by Jannis Leidel

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

comment:29 Changed 4 years ago by Jannis Leidel

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.

Changed 4 years ago by namn

Attachment: ticket2550d.diff added

ticket2550d.diff

comment:30 Changed 4 years ago by namn

Patch needs improvement: unset

Patch revised.

comment:31 Changed 4 years ago by Jannis Leidel <jannis@…>

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