Opened 2 years ago

Closed 2 years ago

#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 :)

Change History (5)

comment:1 Changed 2 years 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 2 years 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 2 years 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 2 years 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 2 years 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!

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