[Django] #34022: admin:logout fails to log out non-staff users
Django
-----------------------------------------+------------------------
Reporter: Jan Pazdziora | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: 4.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/34022>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Jan Pazdziora | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: 4.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jan Pazdziora):
I've filed https://github.com/django/django/pull/16073 with proposed fix.
--
Ticket URL: <https://code.djangoproject.com/ticket/34022#comment:1>
Django
-------------------------------+--------------------------------------
Reporter: Jan Pazdziora | Owner: nobody
Component: contrib.admin | Version: 4.1
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
* status: new => closed
* type: Uncategorized => New feature
* resolution: => wontfix
Comment:
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.
{{{#!python
class CustomAdminSite(AdminSite):
def has_permission(self, request):
return request.user.is_active
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34022#comment:2>
Django
-------------------------------+--------------------------------------
Reporter: Jan Pazdziora | Owner: nobody
Component: contrib.admin | Version: 4.1
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
* status: closed => new
* resolution: wontfix =>
Comment:
Well, the change is backward incompatible in the sense that it changes
behaviour, yes. But it 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34022#comment:3>
Django
-------------------------------+--------------------------------------
Reporter: Jan Pazdziora | Owner: nobody
Component: contrib.admin | Version: 4.1
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
I appreciate you'd like to reopen the ticket, but please
[https://docs.djangoproject.com/en/stable/internals/contributing/triaging-
tickets/#closing-tickets follow the triaging guidelines with regards to
wontfix tickets] and take this to DevelopersMailingList.
--
Ticket URL: <https://code.djangoproject.com/ticket/34022#comment:4>