#18161 closed Bug (wontfix)
Redirection after successful login is not working properly
Reported by: | 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)
Change History (17)
comment:1 by , 13 years ago
Component: | Uncategorized → Generic views |
---|---|
Type: | Uncategorized → Bug |
comment:2 by , 13 years ago
Component: | Generic views → contrib.admin |
---|
comment:3 by , 12 years ago
Easy pickings: | set |
---|---|
Triage Stage: | Unreviewed → 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 by , 12 years ago
Has patch: | set |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Created a pull with the logic proposed by andrewgodwin https://github.com/django/django/pull/135
comment:5 by , 12 years ago
Triage Stage: | Ready for checkin → 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 by , 12 years ago
Triage Stage: | Accepted → 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 by , 12 years ago
Needs tests: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
If it doesn't have tests, it's not RFC.
comment:8 by , 12 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:9 by , 12 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 , 12 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/')
comment:11 by , 12 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 , 12 years ago
comment:13 by , 11 years ago
Resolution: | → invalid |
---|---|
Status: | assigned → 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 by , 11 years ago
Resolution: | invalid |
---|---|
Status: | closed → 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 by , 11 years ago
Resolution: | → wontfix |
---|---|
Status: | new → 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.
IIRC this is a problem with the login implementation in the admin; it isn't related to the generic views.