Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#34022 closed New feature (wontfix)

admin:logout fails to log out non-staff users

Reported by: Jan Pazdziora Owned by: nobody
Component: contrib.admin Version: 4.1
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The Django 4.1 release notes at https://docs.djangoproject.com/en/4.1/releases/4.1/ show that the expected way to log out users is using admin:logout:

<form id="logout-form" method="post" action="{% url 'admin:logout' %}">
  {% csrf_token %}
  <button type="submit">{% translate "Log out" %}</button>
</form>

However, when the logged-in user does not have the is_staff attribute because it used a custom non-admin login, using such approach leads to a redirect back to /admin/ and /admin/login/?next=/admin/ and message

You are authenticated as bob, but are not authorized to access this page.
Would you like to login to a different account?

Since the user tries to log out, meaning strip themselves of any permissions, the authorization check that is currently in place for logout is likely wrong.

The behaviour change happened in https://github.com/django/django/commit/1f84630c87f8032b0167e6db41acaf50ab710879. The original code did

        # The 'logout' view doesn't require that the person is logged in.
        if url == 'logout':
            return self.logout(request)

        # Check permission to continue or display login form.
        if not self.has_permission(request):
            return self.login(request)

which the new code changed to

            url(r'^logout/$',
                wrap(self.logout),
                name='%sadmin_logout'),

with that wrap() around self.logout. So that refactoring change also changed the semantics of the logout behaviour

Change History (4)

comment:1 by Jan Pazdziora, 2 years ago

I've filed https://github.com/django/django/pull/16073 with proposed fix.

comment:2 by Mariusz Felisiak, 2 years ago

Resolution: wontfix
Status: newclosed
Type: UncategorizedNew feature

Thanks for the report, however the proposed change is backward incompatible and introduces a regression (see #159 and 03eeb020a00dedba2594326bd606d8b41d51e80f). You should be able to use a custom AdminSite and overwrite AdminSite.has_permission(), e.g.

class CustomAdminSite(AdminSite):
    def has_permission(self, request):
        return request.user.is_active

comment:3 by Jan Pazdziora, 2 years ago

Resolution: wontfix
Status: closednew

Well, the change is backward incompatible in the sense that it changes behaviour, yes. But in my mind it fixes a problem which was introduced by some refactoring which changed behaviour for non-staff users by mistake.

In the https://github.com/django/django/pull/16073#issuecomment-1250690063 comment I've now shown an alternative approach which would only cater to the non-staff case and as shown by the unmodified test_client_logout_url_can_be_used_to_login (just renamed to test_logout_if_not_authenticated to make its purpose more explicit), the behaviour for the unauthenticated case does not change.

Let me reopen this ticket to have some more discussion about the non-staff case which as far as I can see is currently not captured by any tests so that behaviour seems to be undefined.

Last edited 2 years ago by Jan Pazdziora (previous) (diff)

comment:4 by Mariusz Felisiak, 2 years ago

Resolution: wontfix
Status: newclosed

I appreciate you'd like to reopen the ticket, but please follow the triaging guidelines with regards to wontfix tickets and take this to DevelopersMailingList.

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