Code

Opened 2 years ago

Closed 14 months ago

Last modified 14 months ago

#18161 closed Bug (wontfix)

Redirection after successful login is not working properly

Reported by: wolfgang.doll@… Owned by: apollo13
Component: contrib.admin Version: 1.4
Severity: Normal Keywords:
Cc: apollo13 Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Suppose the following situation:

We have the following url config in urls.py

url(r'^tabview/(?P<page>\d+)/$', 'tabview.views.tabview', name='tabview-tabview'),

We have a view with the following signature:

@login_required
def tabview(request, page='1'):
    pass

We have this settings in settings.py

LOGIN_URL = '/admin/login'
LOGOUT_URL = '/admin/logout'

We are not logged in yet.

Now we try to request localhost:8000/tabview/1/.
We get a redirect to localhost:8000/admin/login/?next=tabview/1/. This is ok.
We do a successful login and get a redirect to localhost:8000/admin/login/?next=tabview/1/.
This is a wrong behaviour !

Expected (According the Django Doc) is a redirect to localhost:8000/tabview/1/.

After some investigation we found the reason:

In django.contrib.auth.views.login the new calculated redirect_field_name inside context is overwritten by the old redirect_field_name in extra_context:

context = {
    'form': form,
    redirect_field_name: redirect_to,
    'site': current_site,
    'site_name': current_site.name,
}
if extra_context is not None:
    context.update(extra_context)

With this change we can fix this issue to the expected behaviour:

context = extra_context or {}
context.update( 
    {
        'form': form,
        redirect_field_name: redirect_to,
        'site': current_site,
        'site_name': current_site.name,
    }
)

Attachments (1)

ProtView.zip (13.9 KB) - added by wolfgang.doll@… 20 months ago.
The minimal django project with the test case

Download all attachments as: .zip

Change History (17)

comment:1 Changed 2 years ago by anonymous

  • Component changed from Uncategorized to Generic views
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Type changed from Uncategorized to Bug

comment:2 Changed 2 years ago by aaugustin

  • Component changed from Generic views to contrib.admin

IIRC this is a problem with the login implementation in the admin; it isn't related to the generic views.

comment:3 Changed 2 years ago by andrewgodwin

  • Easy pickings set
  • Triage Stage changed from Unreviewed to Accepted

I think a better patch would be to login() in contrib.admin.sites - that's where the redirect is set to request.get_full_path, and should instead be only be set to that if the 'next' GET param is missing or pointing off-site (for security reasons, it should only redirect to an internal URL).

comment:4 Changed 2 years ago by pvl

  • Has patch set
  • Triage Stage changed from Accepted to Ready for checkin

Created a pull with the logic proposed by andrewgodwin https://github.com/django/django/pull/135

Last edited 2 years ago by pvl (previous) (diff)

comment:5 Changed 2 years ago by aaugustin

  • Triage Stage changed from Ready for checkin to Accepted

Please don't mark your own patches as RFC. Could you have someone else review it, and if it's indeed commit-ready, mark it as RFC again?

comment:6 Changed 22 months ago by dloewenherz

  • Triage Stage changed from Accepted to Ready for checkin

This feature seems to be working as expected with the patch. Marking as RFC.

Should we include tests with this one?

comment:7 Changed 21 months ago by lrekucki

  • Needs tests set
  • Triage Stage changed from Ready for checkin to Accepted

If it doesn't have tests, it's not RFC.

comment:8 Changed 20 months ago by apollo13

  • Cc apollo13 added
  • Owner changed from nobody to apollo13
  • Status changed from new to assigned

comment:9 Changed 20 months ago by wolfgang.doll@…

I created a small django project and a test to verify the fix.
But I'm not able to write the test in a good way.
Maybe someone can improve...

You can find the project here: https://github.com/downloads/wodo/django-ct/ProtView.zip
With this project you should browse to localhost:8000/pv/

-> Redirect to localhost:8000/admin/login/?next=/pv/
-> Login

     username: django
     passwort: 1234

Without the patch -> Redirect to localhost:8000/admin/login/?next=/pv/
With the patch -> Redirect to localhost:8000/pv/

comment:10 Changed 20 months ago by wolfgang.doll@…

What's wrong with this kind of test?
It should proof the fix but fail in line (->).

from django.test import TestCase

class PVTest(TestCase):
        # load the user django and its password
	fixtures = ['initial_data.json', ]

	def test_login(self):

		location = '/pv/'
		response = self.client.get(location)
		self.assertEqual(response.status_code, 302)

		location = response['location']
		response = self.client.get(location)
		self.assertEqual(response.status_code, 200)

		response = self.client.post(location, {'username': 'django', 'password': '1234'})
->		self.assertEqual(response.status_code, 302)

		location = response['location']
		response = self.client.get(location)
		self.assertEqual(response.status_code, 200)
 		self.assertEqual(location, '/pv/') 


Changed 20 months ago by wolfgang.doll@…

The minimal django project with the test case

comment:11 Changed 20 months ago by wolfgang.doll@…

How can a write a test to proof the fix?
It seems the test client has a bug?!
Please take a look:

#---------------
# pv/tests.py
#---------------

from django.test import TestCase
from django.core.urlresolvers import reverse
from urlparse import urlparse

class PVTest(TestCase):
	fixtures = ['initial_data.json', ]

	def test_login(self):

		username = 'django'
		password = '1234'				
		start = reverse('index')
		error = '<p class="errornote">'

		response = self.client.get(start)
		self.assertEqual(response.status_code, 302)

		location = urlparse(response['location'])[2]
		response = self.client.get(location)
		self.assertEqual(response.status_code, 200)

		response = self.client.post(location, {'username': username, 'password': password})
		content = response.content
		if error in content:
			print 'Error: ' + content.partition(error)[2].partition('</p>')[0].strip()
		self.assertEqual(response.status_code, 302)

		location = urlparse(response['location'])[2]
		response = self.client.get(location)
		self.assertEqual(location, start)
		self.assertEqual(response.status_code, 200)

After the post request django complain about:

Error: Please log in again, because your session has expired.

It seems, the post request will not generate the same result as

self.client.login(username, password)

With this i will get a session, but no redirect.
But getting the redirect after the post request is exactly what is expected.
When we do the steps manually with a real django site all things will be happen as written in the tests.py

NB: Using another password django complaining : "Please enter the correct username and password..."
With this in mind, from my point of view, the post request is working well.

comment:12 Changed 20 months ago by wolfgang.doll@…

After a first look to #11475, #10899, #15740: i'm wondering, if the test client is the right tool to test this fix?
I will give selenium a chance ...

comment:13 Changed 14 months ago by ramiro

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

The admin login code has no concrete URL, reading django/contrib/admin/options.py shows that for every request that lies under the admin site instance URL hierarchy the code there checks if the user is logged in and if she isn't then the login() is called (a Python method call, no HTTP redirection. There isn't entry in any URL map for it to be accessed directly from HTTP clients.)

So, I don't think having LOGIN_URL = '/admin/login' makes any sense and trying to use the admin login form as a project wide login isn't a use case we need to support with code. In particular because the '/login' suffix has no special meaning and the login form is being shown simply as a side effect of trying to access an arbitrary URL delegated to the AdminSite.

I'm closing this ticket as invalid. Please reopen if you disagree

comment:14 Changed 14 months ago by wolfgang.doll@…

  • Resolution invalid deleted
  • Status changed from closed to new

Please test the patch and you will see is is working and in my opinion it is a conveniance way to provide a project wide login page out f the box.
The change is minimal, with a huge effect.

comment:15 Changed 14 months ago by carljm

  • Resolution set to wontfix
  • Status changed from new to closed

I agree with ramiro.

I think the way the admin login works is odd; it would be better if it had its own dedicated URL and used redirects and a ?next parameter (the way a django.contrib.auth login page typically works), rather than displaying the login form directly at any admin URL. A ticket/patch to change that behavior fully I would find interesting (and would also have the side effect of making the admin login more easily reusable as a sitewide login in simple cases).

But this patch is a half-measure that makes no sense within the context of how the admin login is intended to work, it's only useful if you're abusing the admin login by accessing a URL that doesn't actually exist ('/admin/login/' - may as well be '/admin/foobar/', it would work the same) and then expecting to be able to redirect somewhere else from there.

comment:16 Changed 14 months ago by wolfgang.doll@…

Ok, I agree.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.