Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#34032 closed Cleanup/optimization (wontfix)

Base authentication Backend should raise NotImplemented on needed methods

Reported by: Dre Westcook Owned by: piyushdivyankar1994
Component: contrib.auth Version: 4.0
Severity: Normal Keywords: authentication
Cc: Vishal Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Dre Westcook)

Hi all,

Recently I've been trying my hand at creating alternative sign on methods for a django system and I've found the whole process fairly clean.

However I did reach bit of a time waste when my "code that should work, doesn't" -- in my login view, I would authenticate() and login() properly, but with a redirect response I would be an AnonymousUser immediately after.

After two days of debugging and re-reading docs, I found that I missed out a fairly critical sentence: "Authentication backends implements two required methods". -- my authentication backend (of which I was replacing the default) - did not implement get_user() so we would use the default BaseBackend.get_user() which is to return None.

To me, it wasn't quite obvious why the authentication system needs to implement get_user ( as i'd want to just get the user by pk like any other) so this was a little bit of time wasting that I feel could be made a bit more obvious.

Some ideas:

Happy for some thoughts/feedback/pushback. This was just a painpoint for me while developing.

Perhaps it needs to be highlighted in the documentation?

Change History (8)

comment:1 by Dre Westcook, 2 years ago

Description: modified (diff)

comment:2 by piyushdivyankar1994, 2 years ago

Owner: changed from nobody to piyushdivyankar1994
Status: newassigned

comment:3 by piyushdivyankar1994, 2 years ago

I have made the following changes

diff --git a/django/contrib/auth/backends.py b/django/contrib/auth/backends.py
index 4adcf35051..b73d1f9f57 100644
--- a/django/contrib/auth/backends.py
+++ b/django/contrib/auth/backends.py
@@ -11,10 +11,12 @@ UserModel = get_user_model()

 class BaseBackend:
     def authenticate(self, request, **kwargs):
-        return None
+        raise NotImplementedError(
+            "This .authenticate(self, request, **kwargs) must be implemented."
+        )

     def get_user(self, user_id):
-        return None
+        raise NotImplementedError("This .get_user(self, ...) must be implemented.")

     def get_user_permissions(self, user_obj, obj=None):
         return set()

I am unsure if this is what is required.

in reply to:  3 comment:4 by Vishal, 2 years ago

Replying to piyushdivyankar1994:

I have made the following changes

diff --git a/django/contrib/auth/backends.py b/django/contrib/auth/backends.py
index 4adcf35051..b73d1f9f57 100644
--- a/django/contrib/auth/backends.py
+++ b/django/contrib/auth/backends.py
@@ -11,10 +11,12 @@ UserModel = get_user_model()

 class BaseBackend:
     def authenticate(self, request, **kwargs):
-        return None
+        raise NotImplementedError(
+            "This .authenticate(self, request, **kwargs) must be implemented."
+        )

     def get_user(self, user_id):
-        return None
+        raise NotImplementedError("This .get_user(self, ...) must be implemented.")

     def get_user_permissions(self, user_obj, obj=None):
         return set()

I am unsure if this is what is required.

Raised P.R https://github.com/django/django/pull/16086/

comment:5 by Vishal, 2 years ago

Cc: Vishal added
Has patch: set

comment:6 by Mariusz Felisiak, 2 years ago

Easy pickings: unset
Has patch: unset
Resolution: wontfix
Status: assignedclosed
Type: UncategorizedCleanup/optimization

Thanks for the ticket, however BaseBackend methods return None as it's the proper value for invalid credentials, see docs:

"A base class that provides default implementations for all required methods. By default, it will reject any user and provide no permissions."

Unfortunately, both your propositions are backward incompatible and against the current docs. Hope it makes sense.

in reply to:  6 comment:7 by Dre Westcook, 2 years ago

Replying to Mariusz Felisiak:

Thanks for the ticket, however BaseBackend methods return None as it's the proper value for invalid credentials, see docs:

"A base class that provides default implementations for all required methods. By default, it will reject any user and provide no permissions."

Unfortunately, both your propositions are backward incompatible and against the current docs. Hope it makes sense.

Hey thanks for the response. I wanted to have more of a discussion, I wasn't ready to put it in code or even if that was the solution.

my point is the docs seem very unclear, and rely on 3 small words.

Here's some better suggestions, to improve documentation rather than changing code:

The point is that the only time it's mentioned that a backend needs to implement both methods is in that first sentence. Every other code example and explanation does not indicate that both are needed.

Hope that makes sense.

comment:8 by Mariusz Felisiak, 2 years ago

Dre, thanks. Docs improvements are always welcome.

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