Code

Opened 12 months ago

Last modified 11 months ago

#20313 assigned New feature

AnonymousUser should follow custom User implementation

Reported by: thinkingpotato@… Owned by: thinkingpotato
Component: contrib.auth Version:
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Attachments (0)

Change History (6)

comment:1 Changed 12 months ago by thinkingpotato

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Forget the change in django.db.models.sql.where.WhereNode - completely out of topic.

comment:2 Changed 12 months ago by thinkingpotato

  • Owner changed from nobody to thinkingpotato
  • Status changed from new to assigned

comment:4 follow-up: Changed 12 months ago 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.

comment:5 Changed 11 months ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

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)

comment:6 in reply to: ↑ 4 Changed 11 months ago by thinkingpotato

Replying to 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 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. One sentence in the docs. Everyone who did object programming will understand it.

Last edited 11 months ago by thinkingpotato (previous) (diff)

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from thinkingpotato to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.