Code

Opened 8 years ago

Closed 20 months ago

#2550 closed New feature (fixed)

Allow a Backend to Globally Fail Authentication

Reported by: umbrae@… Owned by: aashu_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@… 8 years ago.
Patch to implement 'required' functionality
auth-__init__-changed-viewable.diff (917 bytes) - added by umbrae@… 8 years ago.
A (hopefully) viewable version of the previous diff
patch.diff (3.3 KB) - added by aashu_dwivedi 3 years ago.
the implementation along with test cases submitted by aashu_dwivedi
patch_corrected.diff (4.3 KB) - added by aashu_dwivedi 3 years ago.
corrected patch
ticket2550.diff (4.4 KB) - added by lrekucki 3 years ago.
Fixed indentation in the previous patch. Tweaked the docs a bit.
ticket2550b.diff (4.4 KB) - added by dan.julius@… 3 years ago.
Fixed value of supports_anonymous_user and supports_object_permissions
ticket2550c.diff (4.3 KB) - added by danielr 2 years ago.
Update patch to apply against 1.4
ticket2550d.diff (4.9 KB) - added by namn 2 years ago.
ticket2550d.diff

Download all attachments as: .zip

Change History (39)

Changed 8 years ago by umbrae@…

Patch to implement 'required' functionality

Changed 8 years ago by umbrae@…

A (hopefully) viewable version of the previous diff

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

  • Triage Stage changed from Unreviewed to Design decision needed

comment:3 Changed 7 years ago by jacob

  • Resolution set to fixed
  • Status changed from new to closed
  • Triage Stage changed from Design decision needed to Accepted

comment:4 Changed 7 years ago by gwilson

  • Resolution fixed deleted
  • Status changed from closed to 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 Changed 6 years ago by SmileyChris

  • Keywords easy-pickings added
  • Triage Stage changed from Accepted to 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 Changed 4 years ago by mtredinnick

  • Triage Stage changed from Design decision needed to 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 Changed 4 years ago by aashu_dwivedi

  • Owner changed from nobody to aashu_dwivedi
  • Status changed from reopened to new

Changed 3 years ago by aashu_dwivedi

the implementation along with test cases submitted by aashu_dwivedi

comment:8 Changed 3 years ago by aashu_dwivedi

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

comment:9 Changed 3 years ago by aashu_dwivedi

  • Triage Stage changed from Accepted to Ready for checkin

comment:10 Changed 3 years ago by lrekucki

  • Needs documentation set
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to 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).
Last edited 3 years ago by lrekucki (previous) (diff)

Changed 3 years ago by aashu_dwivedi

corrected patch

comment:11 Changed 3 years ago by aashu_dwivedi

  • Triage Stage changed from Accepted to Ready for checkin

Added corrected patch with the required addition to the documentation .

Changed 3 years ago by lrekucki

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

comment:12 Changed 3 years ago by lrekucki

  • milestone set to 1.4
  • Needs documentation unset
  • Patch needs improvement unset
  • Triage Stage changed from Ready for checkin to 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 Changed 3 years ago by gabrielhurley

  • Component changed from Contrib apps to contrib.auth

comment:14 Changed 3 years ago by aaugustin

#15716 was a duplicate.

comment:15 Changed 3 years ago by lrekucki

  • Type changed from enhancement to New feature

comment:16 Changed 3 years ago by lrekucki

  • Severity changed from normal to Normal

comment:17 Changed 3 years ago by jacob

  • Easy pickings set

comment:18 Changed 3 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 3 years ago by apollo13

  • Cc florian+django@… added

Changed 3 years ago by dan.julius@…

Fixed value of supports_anonymous_user and supports_object_permissions

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

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to 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 Changed 3 years ago by carljm

  • Triage Stage changed from Design decision needed to Accepted

comment:22 Changed 3 years ago by anonymous

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

comment:23 Changed 3 years ago by aaugustin

  • Resolution fixed deleted
  • Status changed from closed to reopened

Revert apparent spam.

Version 0, edited 3 years ago by aaugustin (next)

comment:24 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:25 follow-up: Changed 2 years ago by myusuf3

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

comment:26 in reply to: ↑ 25 Changed 2 years ago by carljm

  • Resolution fixed deleted
  • Status changed from closed to reopened

Please don't mark tickets fixed without explanation.

Changed 2 years ago by danielr

Update patch to apply against 1.4

comment:27 Changed 2 years ago by danielr

  • Triage Stage changed from Accepted to Ready for checkin

comment:28 Changed 2 years ago by jezdez

  • Summary changed from [patch] Allow a Backend to Globally Fail Authentication to Allow a Backend to Globally Fail Authentication

comment:29 Changed 2 years ago by jezdez

  • 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 2 years ago by namn

ticket2550d.diff

comment:30 Changed 2 years ago by namn

  • Patch needs improvement unset

Patch revised.

comment:31 Changed 20 months ago by Jannis Leidel <jannis@…>

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

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.