Opened 10 years ago

Closed 9 years ago

#3032 closed defect (fixed)

AnonymousUser has no attribute is_staff/is_superuser (django.contrib.auth)

Reported by: Simon Greenhill <dev@…> Owned by: Ilya Semenov
Component: Contrib apps Version: master
Severity: normal Keywords: authentication, auth, anonymous
Cc: dev@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

This raises a traceback if some attempts to use is_staff/is_superuser when the user is not logged in (i.e. AnonymousUser is the class).

People probably shouldn't be directly using these (but e.g. checking is_authenticated first, before trying to access these), however this certainly shouldn't raise an error like this.

Index: models.py
===================================================================
--- models.py   (revision 4065)
+++ models.py   (working copy)
@@ -313,3 +313,9 @@
 
     def is_authenticated(self):
         return False
+
+    def is_staff(self):
+        return False
+        
+    def is_superuser(self):
+        return False
\ No newline at end of file

This fix is trivial, but a far better solution than trying to keep these in sync. in future, would be to have an e.g. BaseUser class which is extended by both User and AnonymousUser

Attachments (2)

anonymous-user-properties.diff (2.3 KB) - added by Ilya Semenov 9 years ago.
anonymous-user-properties.2.diff (3.0 KB) - added by Ilya Semenov 9 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 10 years ago by Simon Greenhill <dev@…>

& a similar problem with AnonymousUser raising a NotImplementError if you try to get groups (user.groups.all())

comment:2 Changed 10 years ago by Chris Beaven

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Probably worth fixing the groups issue too then.

comment:3 Changed 9 years ago by Grzegorz Lukasik

Please fix it.

comment:4 Changed 9 years ago by James Bennett

I'm not convinced that there's an issue here; code which doesn't know whether it's been handed a "real" user or an AnonymousUser should always be checking is_authenticated() before doing anything which assumes the user is authenticated.

comment:5 Changed 9 years ago by Grzegorz Lukasik

Something has to be fixed here. From documentation http://www.djangoproject.com/documentation/authentication/:

django.contrib.auth.models.AnonymousUser is a class that implements the django.contrib.auth.models.User interface, with these differences: ...

So documentation should be changed or AnonymousUser should implement all the methods and fields.

comment:6 in reply to:  5 Changed 9 years ago by Marc Fargas <telenieko@…>

Summary: [patch] AnonymousUser has no attribute is_staff/is_superuser (django.contrib.auth)AnonymousUser has no attribute is_staff/is_superuser (django.contrib.auth)

Replying to hauser:

Something has to be fixed here. From documentation http://www.djangoproject.com/documentation/authentication/:

django.contrib.auth.models.AnonymousUser is a class that implements the django.contrib.auth.models.User interface, with these differences: ...

So documentation should be changed or AnonymousUser should implement all the methods and fields.

It also says:

set_password(), check_password(), save(), delete(), set_groups() and set_permissions() raise NotImplementedError.

So maybe we should add to this list every possible method from User (yes: .objects, .get_profile() and so on) or simply say:

This class only implements .is_anonymous() and is_authenticated() and .has_perm() so you should first check .is_authenticated if you plan to use any other function from User, do not assume that AnonymousUser has everything User has.

Maybe .is_staff and .is_superuser could be implemented to be a bit more DRY (if you want to place an "admin" link in a site it doesn't make sense to place two 'if' blocks:)

   {% if user.is_authenticated %}{% is user.is_staff %}<link to admin>{% endif %}{% endif %}

On the other methods, for me, it doesn't seem a good idea as AnonymousUser is not a model and User is, so not all methods make sense.

Comment on this to make a docs patch (the easiest, hence my favourites :P), or decide whether to implement all methods from User...

comment:7 Changed 9 years ago by Thomas Güttler <hv@…>

Hi,

I think AnonymousUser should have an is_superuser attribute like real users.

The above patch inserts a method (not an attribute). Attention: then " is u.is_superuser" is allways
true!.

comment:8 Changed 9 years ago by Ilya Semenov

Owner: changed from nobody to Ilya Semenov
Patch needs improvement: unset
Status: newassigned
Triage Stage: AcceptedDesign decision needed

Marc Fargas, your example is incorrect - placing two {% if %} blocks is not required (a sole internal {% if user.is_staff %} works just fine). Template engine silently skips non-existent attributes/methods calls (see http://www.djangoproject.com/documentation/templates/#variables).

As for the python code which would fail at request.user.is_staff if the user is not logged on, the current behaviour is properly documented.
If we want to change that behaviour, we need a design decision here.

I'm attaching a better patch, which uses properties instead of member methods, as per User model contract. It also has groups and user_permissions property implemented, using new class django.db.models.managers.EmptyManager (which naming conforms to already-existent django.db.models.query.EmptyQuerySet). The documentation is updated to reflect the changes.

Changed 9 years ago by Ilya Semenov

Changed 9 years ago by Ilya Semenov

comment:9 Changed 9 years ago by Ilya Semenov

Keywords: anonymous added
Triage Stage: Design decision neededReady for checkin

Added tests.

comment:10 Changed 9 years ago by Malcolm Tredinnick

Resolution: fixed
Status: assignedclosed

(In [6299]) Fixed #3032 -- Added some useful methods and attributes so that AnonymousUser can proxy for a User a bit more logically. Patch from semenov.

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