Code

#21067 closed Cleanup/optimization (invalid)

is_staff shouldn't be checked in admin templates

Reported by: glarrain Owned by: nobody
Component: contrib.admin Version: master
Severity: Normal Keywords: admin
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In all the templates under django.contrib.admin, the only place where user.is_staff is used is base.html

/django/django/contrib/admin/templates$ grep -r is_staff
admin/base.html:        {% if user.is_active and user.is_staff %}

https://github.com/django/django/blob/master/django/contrib/admin/templates/admin/base.html#L27

This block wraps

        <div id="user-tools">
            {% trans 'Welcome,' %}
            <strong>{% firstof user.get_short_name user.get_username %}</strong>.
            {% block userlinks %}
                {% url 'django-admindocs-docroot' as docsroot %}
                {% if docsroot %}
                    <a href="{{ docsroot }}">{% trans 'Documentation' %}</a> /
                {% endif %}
                {% if user.has_usable_password %}
                <a href="{% url 'admin:password_change' %}">{% trans 'Change password' %}</a> /
                {% endif %}
                <a href="{% url 'admin:logout' %}">{% trans 'Log out' %}</a>
            {% endblock %}
        </div>

It's my impression that the condition user.is_staff should be removed from the "if" clause.

There is already Python code in charge of checking user has proper permissions like admin.views.decorators.staff_member_required and admin.sites.AdminSite.has_permission, so the mentioned condition is redundant.

I happened to discover this when building a custom admin site that doesn't require the user to be staff. The current admin implementation prevents an "elegant" customization/extension and forces the developer to replace the template entirely. If not, the user could still log in and use the admin site but wouldn't see the links to change password and log out, because they are wrapped by the aforementioned "if" clause.

PS: have mercy on me, it's my first patch and "real" bug report :)

Attachments (0)

Change History (5)

comment:1 Changed 10 months ago by glarrain

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Created pull request https://github.com/django/django/pull/1595

Test suite results haven't changed

Ran 5974 tests in ...s

OK (skipped=496, expected failures=11)

comment:2 Changed 10 months ago by mjtamlyn

  • Triage Stage changed from Unreviewed to Ready for checkin
  • Type changed from Bug to Cleanup/optimization

Change looks reasonable, I think that test is redundant and it is unhelpful in the context of custom user models, or in general!

I'm not sure that we will need a note in the release notes, I don't think anyone should be relying on this.

comment:3 Changed 10 months ago by timo

I don't see a use for the is_active check either as inactive users can't login. Thoughts?

comment:4 Changed 10 months ago by Tim Graham <timograham@…>

In aeed2cf3b23161f228c8b221e56ea4d8a7cf71aa:

Added a test to show that the user.is_staff check in admin base.html is necessary.

refs #21067

comment:5 Changed 10 months ago by timo

  • Resolution set to invalid
  • Status changed from new to closed

Actually, it appears that the way the templates are currently structured both checks are required. The checks prevent that block from appearing on the login form if a user has either flag removed while he's already logged in. I've added a test which would fail if is_staff is removed. There's already a test for is_active. There may be a better way to structure the templates so we can handle the situation you have in mind. If you are interested in pursuing that, please open a separate ticket, thanks!

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.