Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#11223 closed (fixed)

logout view of authentication broken

Reported by: ttsmith Owned by: lasko
Component: contrib.auth Version: master
Severity: Keywords:
Cc: miracle2k, bmheight@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

In the documentation for the authentication logout view it says that the redirect_field_name, which defaults to 'next', will override the value for next_page. This is not the case. The redirect_field_name works fine when next_page is not defined, but if next_page is defined then its value we be used even if you pass in a value with the redirect_field_name.

For example the following works fine and correctly redirect to http://www.example.com/foo/

route: url(r'^logout/$', 'django.contrib.auth.views.logout', {} , name='logout')
or
route: url(r'^logout/$', 'django.contrib.auth.views.logout', {'redirect_field_name': 'next'}, name='logout')
and
url: http://www.example.com/logout/?next=/foo/

But this doesn't and redirects the http://www.example.com/

route: url(r'^logout/$', 'django.contrib.auth.views.logout', {'next_page': '/'}, name='logout')
or
route: url(r'^logout/$', 'django.contrib.auth.views.logout', {'next_page': '/', 'redirect_field_name': 'next'}, name='logout')
and
url: http://www.example.com/logout/?next=/foo/

Shouldn't the function in '.../django/contrib/auth/views.py' be something like.

def logout(request, next_page=None, template_name='registration/logged_out.html', redirect_field_name=REDIRECT_FIELD_NAME):
    "Logs out the user and displays 'You are logged out' message."
    from django.contrib.auth import logout
    logout(request)
    redirect_to = request.REQUEST.get(redirect_field_name, '')
    if redirect_to:
        return HttpResponseRedirect(redirect_to)
    elif next_page is None:
        return render_to_response(template_name, 
                                  {'title': _('Logged out')}, 
                                  context_instance=RequestContext(request))
    else:
        # Redirect to this page until the session has been cleared.
        return HttpResponseRedirect(next_page or request.path)

Attachments (2)

views.py.diff (1.6 KB) - added by lasko 5 years ago.
views_with_tests.diff (2.6 KB) - added by lasko 5 years ago.
Patch w/ Tests

Download all attachments as: .zip

Change History (12)

comment:1 follow-up: Changed 6 years ago by kmtracey

  • Triage Stage changed from Unreviewed to Accepted

Not sure whether it is the doc or the code that needs to be fixed here. redirect_field_name for the logout view was added in r10332. The documentation was added in r10371 without, as near as I can tell, a ticket (r10332 fixed #10460, a brief scan of the tickets with higher numbers than that listed as being fixed by r10371 doesn't show any that mention auth logout, unless I missed it). I don't know where that doc came from but it doesn't match the code.

It would seem to be simpler and less risky at this point to just change the doc to reflect the code, and say redirect_field_name is only used when next_page is None. I don't see any great advantage to changing the implementation to what the doc says?

comment:2 in reply to: ↑ 1 Changed 6 years ago by ttsmith

The reason I ran into this bug is because I wanted the documentated implementation. I wanted my logout to redirect to the value of 'next_page', in this case my home page, if no value for 'next' is passed through the url instead of going to a logout template. I understand that I could define a different logout template and code a redirection there, but that would be a much like DRY approach. Ticket #10460 wanted to make the logout work more like the login view and right now it doesn't since 'redirect_field_name' doesn't override 'next_page' unlike the login where 'redirect_field_name' overrides settings.LOGIN_REDIRECT_URL.

comment:3 Changed 6 years ago by miracle2k

  • Cc miracle2k added

comment:4 Changed 5 years ago by anonymous

I was about to submit the same bug. I think the right way is the one documented and proposed in this ticket.This will solve a lot of problems. Ticket #12405 is related to the same problem, although with a different approach. I think the best is to chose one of the proposed solution and solve this problem.

Changed 5 years ago by lasko

comment:5 follow-up: Changed 5 years ago by lasko

  • Has patch set
  • Needs documentation set
  • Needs tests set

Added a patch to resolve the issue but it stills needs tests written as well as the Documentation modification.

comment:6 Changed 5 years ago by lasko

  • Owner changed from nobody to lasko
  • Status changed from new to assigned

Changed 5 years ago by lasko

Patch w/ Tests

comment:7 Changed 5 years ago by lasko

  • Needs tests unset

comment:8 in reply to: ↑ 5 Changed 5 years ago by lasko

  • Cc bmheight@… added
  • Needs documentation unset

Replying to lasko:

Added a patch to resolve the issue but it stills needs tests written as well as the Documentation modification.

The attached patch/test implements the documentation as described.
http://docs.djangoproject.com/en/dev/topics/auth/#django.contrib.auth.views.logout

The behavior implements redirect_field_name overriding next_page if the given GET parameter is passed.
No new documentation should be needed.
I have verified the patch against my build and the tests pass with flying colors.

Someone please review the patch and tests and then we can move forward with this.

comment:9 Changed 5 years ago by jezdez

  • Resolution set to fixed
  • Status changed from assigned to closed

In [15706]:

Fixed #11223 -- Fixed logout view to use the 'next' GET parameter correctly as described in the docs, while only allowing redirection to the same host.

comment:10 Changed 5 years ago by jezdez

In [15707]:

[1.2.X] Fixed #11223 -- Fixed logout view to use the 'next' GET parameter correctly as described in the docs, while only allowing redirection to the same host.

Backport from trunk (r15706).

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