Re: [Django] #34032: Base authentication Backend should raise NotImplemented on needed methods

[Django]

#34032: Base authentication Backend should raise NotImplemented on needed 
methods
-+-
 Reporter:  Dre Westcook |Owner:
 Type:   |  piyushdivyankar1994
  Cleanup/optimization   |   Status:  closed
Component:  contrib.auth |  Version:  4.0
 Severity:  Normal   |   Resolution:  wontfix
 Keywords:  authentication   | Triage Stage:
 |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Mariusz Felisiak):

 Dre, thanks. Docs improvements are always welcome.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/010701836e97277c-fe9cc25f-9e7a-4c57-9cd6-140539ceac1d-00%40eu-central-1.amazonses.com.

Re: [Django] #34032: Base authentication Backend should raise NotImplemented on needed methods

[Django]

#34032: Base authentication Backend should raise NotImplemented on needed 
methods
-+-
 Reporter:  Dre Westcook |Owner:
 Type:   |  piyushdivyankar1994
  Cleanup/optimization   |   Status:  closed
Component:  contrib.auth |  Version:  4.0
 Severity:  Normal   |   Resolution:  wontfix
 Keywords:  authentication   | Triage Stage:
 |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Dre Westcook):

 Replying to [comment:6 Mariusz Felisiak]:
 > Thanks for the ticket, however `BaseBackend` methods return `None` as
 it's the proper value for invalid credentials, see
 
[https://docs.djangoproject.com/en/stable/ref/contrib/auth/#django.contrib.auth.backends.BaseBackend
 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:

 * in the docs that you link: `BaseBackend` should mention explicitly that
 `authenticate()` and `get_user()` return none, in the same "list" style as
 `get_user_permissions()` and `get_group_permissions` is described.
 
https://docs.djangoproject.com/en/4.1/ref/contrib/auth/#django.contrib.auth.backends.BaseBackend
 * in the **Writing an authentication backend** , the code example should
 include the `get_user` definition.
 [https://docs.djangoproject.com/en/4.1/topics/auth/customizing/#writing-
 an-authentication-backend].

 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.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/010701836cb5aa84-97a8933f-e655-4038-b739-349fb902-00%40eu-central-1.amazonses.com.

Re: [Django] #34032: Base authentication Backend should raise NotImplemented on needed methods

[Django]

#34032: Base authentication Backend should raise NotImplemented on needed 
methods
-+-
 Reporter:  Dre Westcook |Owner:
 Type:   |  piyushdivyankar1994
  Cleanup/optimization   |   Status:  closed
Component:  contrib.auth |  Version:  4.0
 Severity:  Normal   |   Resolution:  wontfix
 Keywords:  authentication   | Triage Stage:
 |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Mariusz Felisiak):

 * status:  assigned => closed
 * type:  Uncategorized => Cleanup/optimization
 * has_patch:  1 => 0
 * resolution:   => wontfix
 * easy:  1 => 0


Comment:

 Thanks for the ticket, however `BaseBackend` methods return `None` as it's
 the proper value for invalid credentials, see
 
[https://docs.djangoproject.com/en/stable/ref/contrib/auth/#django.contrib.auth.backends.BaseBackend
 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.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/010701836af3fc23-b575f2cb-fc77-401f-9bd3-5ea589352d29-00%40eu-central-1.amazonses.com.

Re: [Django] #34032: Base authentication Backend should raise NotImplemented on needed methods

[Django]

#34032: Base authentication Backend should raise NotImplemented on needed 
methods
-+-
 Reporter:  Dre Westcook |Owner:
 |  piyushdivyankar1994
 Type:  Uncategorized|   Status:  assigned
Component:  contrib.auth |  Version:  4.0
 Severity:  Normal   |   Resolution:
 Keywords:  authentication   | Triage Stage:
 |  Unreviewed
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  1|UI/UX:  0
-+-
Changes (by Vishal):

 * cc: Vishal (added)
 * has_patch:  0 => 1


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/010701836ad8dd64-4aaec9fe-38f1-4d0b-b2a3-22d3d6975d80-00%40eu-central-1.amazonses.com.

Re: [Django] #34032: Base authentication Backend should raise NotImplemented on needed methods

[Django]

#34032: Base authentication Backend should raise NotImplemented on needed 
methods
-+-
 Reporter:  Dre Westcook |Owner:
 |  piyushdivyankar1994
 Type:  Uncategorized|   Status:  assigned
Component:  contrib.auth |  Version:  4.0
 Severity:  Normal   |   Resolution:
 Keywords:  authentication   | Triage Stage:
 |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  1|UI/UX:  0
-+-

Comment (by Vishal):

 Replying to [comment:3 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/

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/010701836accf512-0886ee01-1a6f-48c0-a167-385f9b93490e-00%40eu-central-1.amazonses.com.

Re: [Django] #34032: Base authentication Backend should raise NotImplemented on needed methods

[Django]

#34032: Base authentication Backend should raise NotImplemented on needed 
methods
-+-
 Reporter:  Dre Westcook |Owner:
 |  piyushdivyankar1994
 Type:  Uncategorized|   Status:  assigned
Component:  contrib.auth |  Version:  4.0
 Severity:  Normal   |   Resolution:
 Keywords:  authentication   | Triage Stage:
 |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  1|UI/UX:  0
-+-

Comment (by 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.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018369b9fc85-c2f652dc-7177-43fc-bcdf-d2b591ab26bf-00%40eu-central-1.amazonses.com.

Re: [Django] #34032: Base authentication Backend should raise NotImplemented on needed methods

[Django]

#34032: Base authentication Backend should raise NotImplemented on needed 
methods
-+-
 Reporter:  Dre Westcook |Owner:
 |  piyushdivyankar1994
 Type:  Uncategorized|   Status:  assigned
Component:  contrib.auth |  Version:  4.0
 Severity:  Normal   |   Resolution:
 Keywords:  authentication   | Triage Stage:
 |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  1|UI/UX:  0
-+-
Changes (by piyushdivyankar1994):

 * owner:  nobody => piyushdivyankar1994
 * status:  new => assigned


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018369994f92-d3534b43-cb98-47df-b560-a7516a35803b-00%40eu-central-1.amazonses.com.

Re: [Django] #34032: Base authentication Backend should raise NotImplemented on needed methods

[Django]

#34032: Base authentication Backend should raise NotImplemented on needed 
methods
+--
 Reporter:  Dotrar  |Owner:  nobody
 Type:  Uncategorized   |   Status:  new
Component:  contrib.auth|  Version:  4.0
 Severity:  Normal  |   Resolution:
 Keywords:  authentication  | Triage Stage:  Unreviewed
Has patch:  0   |  Needs documentation:  0
  Needs tests:  0   |  Patch needs improvement:  0
Easy pickings:  1   |UI/UX:  0
+--
Description changed by Dotrar:

Old description:

> 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. but I'm happy for other considerations.
>
> Some ideas I had for changing this that might've saved time in the
> future:
> * `BaseBackend` to implement a simple `get_user_model().objects.get(
> _meta.pk=pk)` - seeming this is the default for most cases (as far as I
> know?)
> * `BaseBackend` to raise `NotImplemented` to force implementors to define
> ''these two required methods'' as that is what is mentioned in the docs
> (https://docs.djangoproject.com/en/4.1/topics/auth/customizing/#:~:text=implements%20two%20required%20methods)
> * anyone requiring the failthrough approach so that one can auth and
> get_user on different backends can just `pass` it
> * something else
>

> Happy for some thoughts /feedback / pushback. I just know that this was a
> painpoint for me and it wasn't obvious where the `AnonymousUser` was
> coming from. Perhaps it's just a documentation change.

New description:

 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:
 * `BaseBackend` to implement a simple `get_user_model().objects.get(
 _meta.pk=pk)` - seeming this is the default for most cases (as far as I
 know?)
 * `BaseBackend` to raise `NotImplemented` to force implementors to define
 ''these two required methods'' as that is what is mentioned in the docs
 
(https://docs.djangoproject.com/en/4.1/topics/auth/customizing/#:~:text=implements%20two%20required%20methods)
 * anyone requiring the failthrough approach so that one can auth and
 get_user on different backends can just `pass` it
 * something else.


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

 Perhaps it needs to be highlighted in the documentation?

--

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/01070183684781be-2bc64ca9-eb92-4d53-924a-44963de05d50-00%40eu-central-1.amazonses.com.