Opened 5 years ago

Closed 5 years ago

#21067 closed Cleanup/optimization (invalid)

is_staff shouldn't be checked in admin templates

Reported by: German Larrain 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


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 %}

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 %}

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 5 years ago by German Larrain

Has patch: set

Created pull request

Test suite results haven't changed

Ran 5974 tests in ...s

OK (skipped=496, expected failures=11)

comment:2 Changed 5 years ago by Marc Tamlyn

Triage Stage: UnreviewedReady for checkin
Type: BugCleanup/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 5 years ago by Tim Graham

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

comment:4 Changed 5 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 5 years ago by Tim Graham

Resolution: invalid
Status: newclosed

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