[Django] #20313: AnonymousUser should follow custom User implementation
Django
----------------------------------+--------------------
Reporter: thinkingpotato@… | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version:
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+--------------------
Introducing custom User classes opened a few new options for handling
authorization logic, e.g.:
{{{
self.request.user.has_purchased(object)
}}}
or as @akaariai mentioned:
{{{
request.user.has_role_in_org(some_org)
}}}
Without being able to define custom AnonymousUser class that follows User
implementation this will not work.
There are some ideas on how to solve that, and the ones discussed are:
* defining {{{anonymous_user_class}}} on {{{UserClass}}} (@akaariai)
* merging {{{User}}} and {{{AnonymousUser}}} (@apollo13)
The current dirty patch uses the same approach as with
{{{get_user_model()}}}:
* django.contrib.auth.get_anonymous_model
* django.conf.global_settings.AUTH_ANONYMOUS_MODEL
and changes in:
* django.contrib.auth.context_processors
* django.db.models.sql.where.WhereNode
--
Ticket URL: <https://code.djangoproject.com/ticket/20313>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: thinkingpotato@… | Owner: nobody
Type: New feature | Status: new
Component: contrib.auth | Version:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by thinkingpotato):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Forget the change in {{{django.db.models.sql.where.WhereNode}}} -
completely out of topic.
--
Ticket URL: <https://code.djangoproject.com/ticket/20313#comment:1>
Django
Reporter: thinkingpotato@… | Owner:
Type: New feature | thinkingpotato
Component: contrib.auth | Status: assigned
Severity: Normal | Version:
Keywords: | Resolution:
Has patch: 0 | Triage Stage:
Needs tests: 0 | Unreviewed
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by thinkingpotato):
* status: new => assigned
* owner: nobody => thinkingpotato
--
Ticket URL: <https://code.djangoproject.com/ticket/20313#comment:2>
Django
Reporter: thinkingpotato@… | Owner:
Type: New feature | thinkingpotato
Component: contrib.auth | Status: assigned
Severity: Normal | Version:
Keywords: | Resolution:
Has patch: 0 | Triage Stage:
Needs tests: 0 | Unreviewed
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by thinkingpotato):
WIP in https://github.com/thinkingpotato/django/tree/ticket_20313
--
Ticket URL: <https://code.djangoproject.com/ticket/20313#comment:3>
Django
Reporter: thinkingpotato@… | Owner:
Type: New feature | thinkingpotato
Component: contrib.auth | Status: assigned
Severity: Normal | Version:
Keywords: | Resolution:
Has patch: 0 | Triage Stage:
Needs tests: 0 | Unreviewed
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by ptone):
The obvious workaround in current code is to just check if user is
authenticated first, as documented here:
https://docs.djangoproject.com/en/dev/topics/auth/default/#authentication-
in-web-requests
If the custom user method you are calling returns a value that you are
checking truthiness on, then you can `and` it with is_authenticated using
python's logic shortcutting.
Adding one line, and one level of indent seems reasonable amount of effort
given the alternative of having to define an anonymous mirror class for
your custom user.
This has the advantage of keeping it even more explicit in you code when
you are doing something with an authenticated user.
Is there something more than code conciseness that would be enabled by
setting up a custom anon user? Perhaps I'm missing some of the argument in
favor of doing this.
--
Ticket URL: <https://code.djangoproject.com/ticket/20313#comment:4>
Django
Reporter: thinkingpotato@… | Owner:
Type: New feature | thinkingpotato
Component: contrib.auth | Status: assigned
Severity: Normal | Version:
Keywords: | Resolution:
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by russellm):
* stage: Unreviewed => Accepted
Comment:
Accepting on the basis that there is something that needs to be addressed
here. @ptone makes a good point about checking is_authenticated first, but
I can see how being able to "just use it" could be useful under some
circumstances, and we've been encouraging that behaviour in the days of
non-swappable User models.
Personally, I'm not completely sold that get_anonymous_model() or
ANONYMOUS_USER_MODEL is needed here - that feels a bit like overkill.
However, I'm not sure I've got a substantially better option, other than
some sort of implicit contract about has_* methods returning True (or
something similar)
--
Ticket URL: <https://code.djangoproject.com/ticket/20313#comment:5>
Django
Reporter: thinkingpotato@… | Owner:
Type: New feature | thinkingpotato
Component: contrib.auth | Status: assigned
Severity: Normal | Version:
Keywords: | Resolution:
Has patch: 0 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by thinkingpotato):
Replying to [comment:4 ptone]:
> The obvious workaround in current code is to just check if user is
authenticated first, as documented here:
> Is there something more than code conciseness that would be enabled by
setting up a custom anon user? Perhaps I'm missing some of the argument in
favor of doing this.
Let's say you ask an outsourced programmer to write a module that will
enable some kind of model-level authorization mechanism or whatever
requiring user based on current request. And that programmer has no idea
about Django internals. All you define is the interface you both will be
using, which is quite straigthforward: passing MyUser instance as the
first argument. But suddenly it's not MyUser instance anymore,
```request.user``` becomes a AnonymousUser that is substantially different
class, implementing different methods and behaviour. Moreover, you can't
even change that. It just makes no sense. Code conciseness, even if was
the only reason for solving this issue, in my opinion is the most
important one to let people do their stuff easily and logically. Speaking
of my personal preferences, I would rather put one line in my custom User
class, rather than repeat ```is_authenticated``` ~80 times (after looking
at my recent code).
Replying to [comment:5 russellm]:
> Accepting on the basis that there is something that needs to be
addressed here. @ptone makes a good point about checking is_authenticated
first, but I can see how being able to "just use it" could be useful under
some circumstances, and we've been encouraging that behaviour in the days
of non-swappable User models.
>
> Personally, I'm not completely sold that get_anonymous_model() or
ANONYMOUS_USER_MODEL is needed here - that feels a bit like overkill.
However, I'm not sure I've got a substantially better option, other than
some sort of implicit contract about has_* methods returning True (or
something similar)
It feels a bit like overkill to me as well, but on the other hand seems
like a price you have to pay for tight integration between different
Django services.
If I understand you correctly, developers would not be able to use methods
starting with a keyword reserved by Django. Personally, I would be scared
if framework interfered my design decisions so much. On the other hand, I
bet that the first day after introducing it #django IRC would be flooded
with questions on how to use it. Defining a class is clean and simple.
Once sentence in the docs. Everyone who did object programming will
understand it.
--
Ticket URL: <https://code.djangoproject.com/ticket/20313#comment:6>
Django
Reporter: thinkingpotato@… | Owner:
Type: New feature | thinkingpotato
Component: contrib.auth | Status: assigned
Severity: Normal | Version:
Keywords: | Resolution:
Has patch: 0 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* cc: julenx@… (added)
Comment:
We basically have the same issue but would need to have a different
solution, since we're using a special user under a reserved username which
is always considered to be anonymous.
Perhaps an `ANONYMOUS_USER` attribute in the `AUTH_USER_MODEL` model could
define what an anonymous user is: a string that matches a value for
`USERNAME_FIELD`, a specific instance of a class etc.
--
Ticket URL: <https://code.djangoproject.com/ticket/20313#comment:7>
Django
Reporter: thinkingpotato@… | Owner:
Type: New feature | thinkingpotato
Component: contrib.auth | Status: assigned
Severity: Normal | Version:
Keywords: | Resolution:
Has patch: 0 | Triage Stage: Accepted
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by thinkingpotato):
This is actually a very neat idea: to create a common interface for all
use cases. Assigning a function that returns whatever is needed would be a
nice addition! +1
--
Ticket URL: <https://code.djangoproject.com/ticket/20313#comment:8>
Django
Reporter: thinkingpotato@… | Owner:
| thinkingpotato
Type: New feature | Status: assigned
Component: contrib.auth | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by skorokithakis):
I've hit this problem as well. I'm trying to add a property to each user
and reference it in the template, with a default for an anonymous user,
and it's much, much harder since I can't add the same method to the
anonymous user as the authenticated user.
However, I'm not really sure why Django even has two classes for this.
Wouldn't an anonymous user be a regular `User` object for which
`is_anonymous()` would return `True`? `is_anonymous()` could decide this
any way it liked (lack of `id`, lack of some specific property, or even
just `_anonymous` set to True).
That way, all methods would distinguish between authenticated and
unauthenticated users through the standard interface, i.e. the
`is_anonymous()` method (and its converse, `is_authenticated()`). Does
that make sense?
--
Ticket URL: <https://code.djangoproject.com/ticket/20313#comment:9>
Django
Reporter: thinkingpotato@… | Owner:
| thinkingpotato
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Luc Saffre):
* Attachment "patch-20313-20170608.diff" added.
A solution which works for me (using branch 1.11rc1) and maybe works for
everybody but needs more work to be complete
Django
Reporter: thinkingpotato@… | Owner:
| thinkingpotato
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Sergey Fedoseev):
* cc: Sergey Fedoseev (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20313#comment:10>
Django
Reporter: thinkingpotato@… | Owner:
| thinkingpotato
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tobias Wiese):
* cc: Tobias Wiese (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20313#comment:11>
Django
Reporter: thinkingpotato@… | Owner:
| thinkingpotato
Type: New feature | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Changes (by Evstifeev Roman):
* cc: Evstifeev Roman (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20313#comment:12>