Opened 12 years ago

Last modified 7 weeks 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: julenx@…, Sergey Fedoseev, Tobias Wiese, Evstifeev Roman 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 (1)

patch-20313-20170608.diff (7.6 KB ) - added by Luc Saffre 7 years ago.
A solution which works for me (using branch 1.11rc1) and maybe works for everybody but needs more work to be complete

Download all attachments as: .zip

Change History (13)

comment:1 by thinkingpotato, 12 years ago

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

comment:2 by thinkingpotato, 12 years ago

Owner: changed from nobody to thinkingpotato
Status: newassigned

comment:4 by Preston Holmes, 12 years ago

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 by Russell Keith-Magee, 11 years ago

Triage Stage: UnreviewedAccepted

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)

in reply to:  4 comment:6 by thinkingpotato, 11 years ago

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 years ago by thinkingpotato (previous) (diff)

comment:7 by julen, 11 years ago

Cc: julenx@… added

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.

comment:8 by thinkingpotato, 11 years ago

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

comment:9 by Stavros Korokithakis, 9 years ago

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?

by Luc Saffre, 7 years ago

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

comment:10 by Sergey Fedoseev, 6 years ago

Cc: Sergey Fedoseev added

comment:11 by Tobias Wiese, 5 years ago

Cc: Tobias Wiese added

comment:12 by Evstifeev Roman, 7 weeks ago

Cc: Evstifeev Roman added
Note: See TracTickets for help on using tickets.
Back to Top