Redirection after successful login is not working properly

[wolfgang.doll@…]

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,
    }
)

[Aymeric Augustin]

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

[Andrew Godwin]

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

[Pedro Lima]

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

[Aymeric Augustin]

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?

[Dan Loewenherz]

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

Should we include tests with this one?

[Łukasz Rekucki]

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

[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/

[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/') 

[wolfgang.doll@…]

The minimal django project with the test case

[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.

[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 ...

[Ramiro Morales]

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

[wolfgang.doll@…]

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.

[Carl Meyer]

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.

[wolfgang.doll@…]

Ok, I agree.