Opened 8 years ago

Closed 8 years ago

#3032 closed defect (fixed)

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

Reported by: Simon Greenhill <dev@…> Owned by: 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 semenov 8 years ago.
anonymous-user-properties.2.diff (3.0 KB) - added by semenov 8 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 8 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 8 years ago by SmileyChris

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

Probably worth fixing the groups issue too then.

comment:3 Changed 8 years ago by hauser

Please fix it.

comment:4 Changed 8 years ago by ubernostrum

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 follow-up: Changed 8 years ago by 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.

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

  • Summary changed from [patch] AnonymousUser has no attribute is_staff/is_superuser (django.contrib.auth) to 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 8 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 8 years ago by semenov

  • Owner changed from nobody to semenov
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Triage Stage changed from Accepted to Design 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 8 years ago by semenov

Changed 8 years ago by semenov

comment:9 Changed 8 years ago by semenov

  • Keywords anonymous added
  • Triage Stage changed from Design decision needed to Ready for checkin

Added tests.

comment:10 Changed 8 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from assigned to closed

(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