is_staff shouldn't be checked in admin templates

[German Larrain]

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

[German Larrain]

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)

[Marc Tamlyn]

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.

[Tim Graham]

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

[Tim Graham <timograham@…>]

In aeed2cf3b23161f228c8b221e56ea4d8a7cf71aa:

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

refs #21067

[Tim Graham]

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!