Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#18161 closed Bug (wontfix)

Redirection after successful login is not working properly

Reported by: wolfgang.doll@… Owned by: Florian Apolloner
Component: contrib.admin Version: 1.4
Severity: Normal Keywords:
Cc: Florian Apolloner 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@… 11 years ago.
The minimal django project with the test case

Download all attachments as: .zip

Change History (17)

comment:1 by anonymous, 12 years ago

Component: UncategorizedGeneric views
Type: UncategorizedBug

comment:2 by Aymeric Augustin, 12 years ago

Component: Generic viewscontrib.admin

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

comment:3 by Andrew Godwin, 12 years ago

Easy pickings: set
Triage Stage: UnreviewedAccepted

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 by Pedro Lima, 12 years ago

Has patch: set
Triage Stage: AcceptedReady for checkin

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

Version 0, edited 12 years ago by Pedro Lima (next)

comment:5 by Aymeric Augustin, 12 years ago

Triage Stage: Ready for checkinAccepted

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 by Dan Loewenherz, 12 years ago

Triage Stage: AcceptedReady 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 by Łukasz Rekucki, 11 years ago

Needs tests: set
Triage Stage: Ready for checkinAccepted

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

comment:8 by Florian Apolloner, 11 years ago

Cc: Florian Apolloner added
Owner: changed from nobody to Florian Apolloner
Status: newassigned

comment:9 by wolfgang.doll@…, 11 years ago

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 by wolfgang.doll@…, 11 years ago

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


by wolfgang.doll@…, 11 years ago

Attachment: ProtView.zip added

The minimal django project with the test case

comment:11 by wolfgang.doll@…, 11 years ago

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 by wolfgang.doll@…, 11 years ago

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 by Ramiro Morales, 11 years ago

Resolution: invalid
Status: assignedclosed

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 by wolfgang.doll@…, 11 years ago

Resolution: invalid
Status: closednew

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 by Carl Meyer, 11 years ago

Resolution: wontfix
Status: newclosed

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 by wolfgang.doll@…, 11 years ago

Ok, I agree.

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