#10061 closed (fixed)
incorrect logout link in admin
Reported by: | Tim Miller | Owned by: | Malcolm Tredinnick |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Keywords: | ||
Cc: | jarek.zgoda@…, David Larlet, mgarcia@…, sebastian.rahlf@…, djjordaan@…, idan@…, xian@…, Sergey Belov, mikko@…, django@…, siddhartag@…, rodrigo@…, grf@…, DrMeers@…, mariano.mara@…, cmawebsite@…, uptimebox@…, andy@…, dtamborelli@…, beathan@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | yes |
Needs tests: | yes | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
I'm using revision 9772 and the development server. After starting a new project, activating the admin interface and logging in.. admin displays an incorrect logout link.
http://127.0.0.1:8000/admin/admin/logout/
Logs out successfully if I remove the duplicate admin.
Attachments (18)
Change History (98)
comment:1 by , 16 years ago
Component: | Uncategorized → django.contrib.admin |
---|
comment:2 by , 16 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 16 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
The issue is with the way root_path is handled, this may require some of the reversing stuff malcolm and I discussed on the mailing list.
by , 16 years ago
Attachment: | admin-urls.2.diff added |
---|
fixed a stupid typo, I've tested this under every conceivable admin setup and it seems to work, the only thing that really needs review(IMO) is the scoping stuff in the url tag
by , 16 years ago
Attachment: | admin-urls.4.diff added |
---|
fixed issues for admin and auth, also cleaned up the URL tag, it shouldn't alter it's own state if not necessary
comment:4 by , 16 years ago
Applying this patch fixes the logout/password change urls but breaks a custom admin view:
class SupportAdmin(admin.AdminSite): def get_urls(self): from django.conf.urls.defaults import patterns, url urls = super(SupportAdmin, self).get_urls() my_urls = patterns('', url(r'^pledge/teamapproach/$', self.admin_view(self.team_approach), name="support_admin_teamapproach"), ) return my_urls + urls def team_approach(self, request, *args, **kwargs): ... context = { 'adminform': selector_form, 'media': selector_form.media, 'admin_site': self.name } return render_to_response("admin/pledge/team_approach_selector.html", context, context_instance=RequestContext(request))
This view wouldn't work unless admin_site was added to the context. Just wanted to mention this change appears to be necessary for any custom admin view
comment:5 by , 16 years ago
You are correct, I want to wait for a bit of review to the patch itself before I add that to the docs, but it should be noted.
by , 16 years ago
Attachment: | admin-urls.5.diff added |
---|
comment:6 by , 16 years ago
Cc: | added |
---|
comment:7 by , 16 years ago
Hello, I have the same error with this test:
from django.test import TestCase from datetime import datetime class PostBugTestCase(TestCase): fixtures = ['tests.json'] def test_stuff(self): from django.test.client import Client c = Client() data = {'question':'test', 'pub_date':datetime.now()} print c.login(username='batiste', password='b') response = c.post('/admin/toto/poll/add/?toto=true', data) #print response.content self.assertRedirects(response, '/admin/toto/poll/')
I applied the patch without luck.
comment:8 by , 16 years ago
Resolution: | → worksforme |
---|---|
Status: | assigned → closed |
Upon patching the logout link now works for me with a second custom admin. I have one at /admin/ and another at /manage/ I tried with both regular users and superuers.
However going to /admin/logout/ (or /manage/logout/) if already logged out returns a sign-in page that when signed into immediately logs out, and returns to /admin/ (/manage/) with the regular login page. That is different than behaviour when running one admin site and a logged out user visits /admin/logout/. That would show the 'successful logout template'.
comment:9 by , 16 years ago
Resolution: | worksforme |
---|---|
Status: | closed → reopened |
sorry, didn't mean to close it
by , 16 years ago
Attachment: | admin-urls.6.diff added |
---|
by , 16 years ago
Attachment: | admin-urls.7.diff added |
---|
by , 16 years ago
Attachment: | admin-urls.8.diff added |
---|
comment:11 by , 16 years ago
Cc: | added |
---|
comment:12 by , 16 years ago
milestone: | → 1.1 |
---|
comment:13 by , 16 years ago
Cc: | added |
---|
comment:14 by , 16 years ago
Cc: | added |
---|
follow-up: 17 comment:15 by , 16 years ago
To fix this problem:
1) Open django_directory/contrib/admin/sites.py.
2) Find "self.root_path = 'admin/'" (around line 41).
3) Replace it with "self.root_path = '/admin/'".
4) Save and restart your app/server.
comment:16 by , 16 years ago
Cc: | added |
---|
comment:17 by , 16 years ago
This does not fix the issue. If i add my admin site on another url like so:
(r'adminxx/', include(admin.site.urls)),
Then the admin url's are wrong again.
Replying to treworld:
To fix this problem:
1) Open django_directory/contrib/admin/sites.py.
2) Find "self.root_path = 'admin/'" (around line 41).
3) Replace it with "self.root_path = '/admin/'".
4) Save and restart your app/server.
by , 16 years ago
Attachment: | admin-urls.9.diff added |
---|
I've attempted my first try at a patch. Here its is. Its much simpler than the other patches. Am i missing something ?
comment:18 by , 16 years ago
Cc: | added |
---|
comment:19 by , 16 years ago
That's a) backwards incompatible, and b) doesn't work with multiple admin sites.
comment:20 by , 16 years ago
Has patch: | set |
---|
follow-up: 23 comment:21 by , 16 years ago
I've had this patch applied for a while now since I've been tracking trunk. Did my weekly update to trunk and was getting errors with urlresolvers.py. I reverted this patch and it fixed the errors.
Also with the latest trunk (with this patch removed), the admin urls are working again. So I think this patch might not be needed.
comment:22 by , 16 years ago
Cc: | added |
---|
comment:23 by , 16 years ago
Replying to dalore:
Also with the latest trunk (with this patch removed), the admin urls are working again. So I think this patch might not be needed.
This problem does still exist in Django trunk r10526.
In case it's helpful to others: the simplest and least invasive temporary workaround is to just add this line in your urls.py:
admin.site.root_path = '/admin/'
If you run the admin at an alternate URL, just use the correct one here; if you run multiple admin instances, you can set the root_path for each one in the same way.
by , 16 years ago
Attachment: | admin-urls.12.diff added |
---|
comment:24 by , 16 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:25 by , 16 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Ready for checkin → Accepted |
admin-urls.12.diff has lost all documentation.
Changes in core/urlresolvers.py and url template tag are not documented and do not have tests.
viewname = ''.join(viewname.split(':'))
is especially ugly. Whole thing looks like dirty hack and this line can be replaced by
viewname = viewname.replace(':', '')
anyway.
Is this ticket really so complicated that it needs special infrastructure in resolvers?
comment:26 by , 16 years ago
Also change in core/urlresolvers.py is backwards-incompatible as someone can use ':' in view names.
comment:27 by , 16 years ago
Owner: | changed from | to
---|---|
Status: | reopened → new |
@dc, you're missing a whole bunch of context here, I suspect. Yes, it will be slightly backwards-incompatible and the colon was chosen for precisely that reason. We need namespaces in URL reversing now. it's basically a bug that it isn't there at the moment. We've chosen a character that is both readable, common to the majority of keyboards and relatively unlikely to appear in names (as opposed to, say, dashes or underscores, which are more commonly used or commas, which are going to be a readability hassle). The infrastructure in place here is designed to handle things a bit more broad than just the admin case.
So, yes, this approach is necessary. The particular line you point at is indeed a bug in the patch.
Since I'm in the neighbourhood now, assigning to me so that I remember to commit this darn thing over the weekend.
comment:28 by , 16 years ago
Maybe i'm wrong, but it looks like that you are inventing namespaces as workaround for url tag inability to work with template variables. I still think that you are breaking butterfly on the wheel (and "just strip colon and reverse" is not a very good thing IMHO). But if core developers really think that this is the best way...
At least document it please.
comment:29 by , 16 years ago
Yes, you are mistaken. That's not why we're using namespaces. Unsurprisingly, it will be documented.
comment:30 by , 16 years ago
ok but how "namespaces" in admin-urls.12.diff differ from for e.g. {% url var1|concat:var2|concat:var3 %} Why do you want to make all this more implicit and fragile?
comment:31 by , 16 years ago
*sigh* Please let's not have design discussions in tickets. Also, give us, the core commiters, some credit. We don't want to make things more implicit and fragile. That's simply a ridiculous assertion. We go for solid code, as well as neatness and usability.
As I noted in my original comment, the replacing of colons by nothing is simply a bug in the patch. It actually leads to ambiguous situations. I doubt that I'll be committing that particular line, for example. That's why patches are uploaded and reviewed with great care and why this ticket has not yet been closed. It's a very subtle area (before you post a comment pointing out that jacob moved it to "ready for checkin" .. yes, I noticed. Reasonable people can disagree on the state and timing of things and I'm sure Jacob would have carefully examined things before finally committing as well.)
Just wait a little bit and this will be resolved, please. There's a long history of the development of this. Search the django-dev archives for a lot fo the design discussion if you really care to look at it.
comment:32 by , 16 years ago
Have misunderstood you. I have nothing against namespaces but I just do not like this particular implementation. That was my point. I believe that you will solve problem in more elegant way. I'm sorry for disturbance.
comment:33 by , 16 years ago
We also didn't lose any docs, jacob just committed them separately(which was really the right way to handle it in the first place).
comment:34 by , 16 years ago
Cc: | added |
---|
comment:35 by , 16 years ago
Cc: | added |
---|
comment:36 by , 15 years ago
Cc: | added |
---|
What about adding something like:
def _get_root_path(self): if not hasattr(self, '_root_path'): self._root_path = reverse('%sadmin_index' % self.name) return self._root_path root_path = property(_get_root_path)
to the AdminSite class?
comment:37 by , 15 years ago
sorry I didn't reed up properly, discard my suggestion, just use it if you want something working now.
comment:38 by , 15 years ago
Cc: | added |
---|
comment:39 by , 15 years ago
Cc: | added |
---|
comment:40 by , 15 years ago
Cc: | added |
---|
comment:41 by , 15 years ago
this problem is still happening in revision 10865, and it's happening with the change password link as well.
if urls.py is setting the admin url to (r'admin/', include(admin.site.urls)), the links are adding an extra "admin/" to the path and breaking because "admin/admin/logout" does not exist.
comment:43 by , 15 years ago
people, you don't want to release 1.1 with this embarassing bug, really!
comment:45 by , 15 years ago
Cc: | added |
---|
by , 15 years ago
Attachment: | admin-urls.13.diff added |
---|
Probably just a whitespace change, but I can't remember if I've had to resolve any merge conflicts
comment:46 by , 15 years ago
Cc: | added |
---|
comment:47 by , 15 years ago
Cc: | removed |
---|
comment:48 by , 15 years ago
Cc: | added |
---|
comment:50 by , 15 years ago
Cc: | added |
---|
comment:51 by , 15 years ago
Cc: | added |
---|
comment:53 by , 15 years ago
Yes - I'm working on it. We know the problem exists, and it needs to be fixed - that's why it's the last blocker on v1.1. The issue is that the fix isn't obvious - Alex's solution is closer to a workaround than a real fix.
I have a patch locally which I'm almost ready to share - there is still a couple of edge cases that I need to resolve.
comment:54 by , 15 years ago
First time I saw this bug, I though was a simple typo/confusion error, I never realized that it would be the last (and probabbly the hardest) one.
BTW, I have 1 year using Python and 6 months using Django (wonderful couple) I hope some day I get the skills to make contributions.
Congratulations.
comment:55 by , 15 years ago
Cc: | added |
---|
by , 15 years ago
Attachment: | t10061-r11201.diff added |
---|
First attempt at a complete namespace lookup fix for redeployed applications
comment:56 by , 15 years ago
Ok - here's a first pass at revised fix for this. No guarantees that this is the final solution, or that details won't change between now and final commit. Feedback welcome.
comment:57 by , 15 years ago
Cc: | added |
---|
follow-up: 61 comment:58 by , 15 years ago
I'm playing around with it a bit on a project that this bug was showing up with. Here are a few notes. Hopefully you find them helpful:
- There's a print statement in django.core.urlresolvers L412
- In django.contrib.admin.sites L235 reverse is used but it's not imported
- Traversing into the /admin/docs/ area I'm getting an error 'function' object has no attribute 'split' at django/core/urlresolvers.py in reverse, line 298
I've fixed each of these in the patch to be posted next. Is a patch of a patch the best here?
follow-ups: 60 62 comment:59 by , 15 years ago
Couples thoughts:
1) I don't see any tests of the admin itself to prove this really works.
2) I don't think Rob's fix for the split() error is correct, reversing a function itself is legal, there should be a conditional arround the view.split() line in the event it isn't a basestring.
3) I'm not sure I follow how {% url admin:logout %} works, as best I can see there's no admin in the context, thus that will always reverse the string "admin:logout" which is only correct in the case where the name of the admin_site is in fact "admin", and thus the reverse fails on custom AdminSites in a multi-adminsite scenario.
comment:60 by , 15 years ago
Replying to Alex:
Couples thoughts:
1) I don't see any tests of the admin itself to prove this really works.
No - it's testing the more general problem - that URL namespace lookup. A test of the admin probably wouldn't go astray as a literal regression for the ticket, but once you have proved 1+1=2, there isn't a whole lot of gain in proving 1+2 = 3.
3) I'm not sure I follow how {% url admin:logout %} works, as best I can see there's no admin in the context, thus that will always reverse the string "admin:logout" which is only correct in the case where the name of the admin_site is in fact "admin", and thus the reverse fails on custom AdminSites in a multi-adminsite scenario.
Did you try the patch, Alex? If you do, you'll find that it works for the case you describe.
You are correct there's no admin mentioned in the public context. That's the point. However, the context is tracking the name of the current admin instance.
The reason this approach works is that admin:logout isn't doing a simple lookup for the namespace 'admin' - it's looking for a registered application called admin. The reverse function is provided an argument (app_name - probably better called 'current_app') that lets the application rendering the template specify the currently active application instance. When rendering the views for 'customadmin', the current app is set to 'customadmin', and so admin:logout is transormed to customadmin:logout, which resolves as /customadmin/logout. If the current app is 'admin', then admin:logout is transformed to admin:logout, which resolves as a /admin/logout.
comment:61 by , 15 years ago
Replying to Rob Hudson <treborhudson@gmail.com>:
I'm playing around with it a bit on a project that this bug was showing up with. Here are a few notes. Hopefully you find them helpful:
- There's a print statement in django.core.urlresolvers L412
- In django.contrib.admin.sites L235 reverse is used but it's not imported
- Traversing into the /admin/docs/ area I'm getting an error 'function' object has no attribute 'split' at django/core/urlresolvers.py in reverse, line 298
Thanks for those pickups, Rob. 1+2 are no brainers; I'll have to check 3 to see that it isn't masking something more sinister.
I've fixed each of these in the patch to be posted next. Is a patch of a patch the best here?
As good as anything.
follow-up: 64 comment:62 by , 15 years ago
Replying to Alex:
Couples thoughts:
2) I don't think Rob's fix for the split() error is correct, reversing a function itself is legal, there should be a conditional arround the view.split() line in the event it isn't a basestring.
What evidence can you provide to support this assertion, Alex? As best as I can make out, the line that Rob has identified has historically fallen through to the NoReverseMatch exception, rather than being handled as a callable. I can't find any example in code or docs where a a reverse lookup is based on a callable. All the usage (documented and actual) that I can see uses reverse(string), which makes sense to me given the purpose of the function.
On top of that, admin.site.root is marked as deprecated, so we would be well served to avoid using it.
by , 15 years ago
Attachment: | t10061-r11201.v2.diff added |
---|
Updated patch including suggestions from Alex and Rob
comment:63 by , 15 years ago
A updated fix - this addresses the issues found by Rob, including some related problems found with the admindocs app. It also adds regression tests for the admin site. The full test suite is still known to throw 18 errors in comment_tests, and 1 failure in admin_widgets. I'm looking into these failures.
comment:64 by , 15 years ago
Replying to russellm:
Replying to Alex:
Couples thoughts:
2) I don't think Rob's fix for the split() error is correct, reversing a function itself is legal, there should be a conditional arround the view.split() line in the event it isn't a basestring.
What evidence can you provide to support this assertion, Alex? As best as I can make out, the line that Rob has identified has historically fallen through to the NoReverseMatch exception, rather than being handled as a callable.
For my next trick, I will put my entire lower leg into my mouth. Apologies, Alex - you are correct: a callable should be allowed to reverse, the docs say so, and so do the comment tests (hence the 18 errors). I'm about to attach a patch that passes the full test suite.
by , 15 years ago
Attachment: | t10061-r11201.v3.diff added |
---|
Third pass - now passing full test suite.
comment:65 by , 15 years ago
Cc: | added |
---|
follow-ups: 67 69 comment:66 by , 15 years ago
Russell, may i suggest using isinstance() for type checking?
Better replace
type(view) == tuple
with
isinstance(view, tuple)
Not critical but more pythonic.
comment:67 by , 15 years ago
Replying to dc:
Russell, may i suggest using isinstance() for type checking?
Better replace
type(view) == tuplewith
isinstance(view, tuple)Not critical but more pythonic.
And more PEP8
follow-up: 70 comment:68 by , 15 years ago
Another possible bug: The password_change_done method on AdminSites reverses using the form reverse('%s:password_change_done' % self.name), while the render method on RelatedFieldWidgetWrapper uses the form reverse('admin:%s_%s_add' % info, app_name=self.admin_site.name). My understanding is that the 2nd form would be correct, however it seems that the term app_name is being used inconsistently, AdminSite instances have both a name and an app_name, here we are passing the name as a parameter named app_name, which is rather confusing.
The get_root_path method will, as far as I can tell, fail using the old admin.site.root form.
Another concern is the use of app_name on Context, I think this attribute should be exclusively on RequestContext, as this is the object that knows about the rest of the Django environ. The only change that needs to be made in light of this is having the URLNode.render use getattr(context, 'app_name', None) in place of context.app_name.
Finally the new form for extending admin urls needs to be documented (as does the namespacing infrastructure probably).
comment:69 by , 15 years ago
Replying to dc:
Russell, may i suggest using isinstance() for type checking?
This was more an accident of history, rather than intent. I've corrected it in my local checkout, and made the check a little more liberal (checking for tuple and list, not just tuple).
comment:70 by , 15 years ago
Replying to Alex:
Another possible bug: The password_change_done method on AdminSites reverses using the form reverse('%s:password_change_done' % self.name), while the render method on RelatedFieldWidgetWrapper uses the form reverse('admin:%s_%s_add' % info, app_name=self.admin_site.name). My understanding is that the 2nd form would be correct,
You are correct. I've fixed this in my local branch.
however it seems that the term app_name is being used inconsistently, AdminSite instances have both a name and an app_name, here we are passing the name as a parameter named app_name, which is rather confusing.
As I noted in comment 60, the app_name argument to Context and reverse should probably be something like 'current_app'. I'll make this change.
The get_root_path method will, as far as I can tell, fail using the old admin.site.root form.
Can you elaborate on the error case here? The only way I can generate problems is if I mix old-style and new-style admin registrations, which is understandable given that the old-form doesn't define (or utilize) the app_name. I don't see any problem with a standard "old-admin registration plus admin docs" setup, or a 'multiple new-admin plus admin docs".
Another concern is the use of app_name on Context, I think this attribute should be exclusively on RequestContext, as this is the object that knows about the rest of the Django environ. The only change that needs to be made in light of this is having the URLNode.render use getattr(context, 'app_name', None) in place of context.app_name.
Hrm.. Not sure I agree here. RequestContext is the context that knows about the request, not necessarily the entire environment (although, as a web app, a lot of the current environment is contained or associated with the request). I don't think there's any examples in admin, but I can see of situations where a simple help page in a redeployable app might return a Context, rather than a RequestContext, but it would still be appropriate to be able to reverse a name to that help page.
Finally the new form for extending admin urls needs to be documented (as does the namespacing infrastructure probably).
Granted. I'm waiting until the syntax settles down before I start writing docs.
I would, however, make some sort of comment about glass houses... the include(object.urls) notation that you contributed wasn't documented until last night, and you didn't write the docs :-)
comment:71 by , 15 years ago
For those interested, I'm pushing my modifications into my public git repository: http://github.com/freakboy3742/django/tree/t10061-rkm
follow-up: 74 comment:72 by , 15 years ago
Russell: (simple nitpick) If the user isn't supposed to modify the app_name
and name
attibutes inside AdminSite.get_urls(), wouldn't it be better to leave it to return an urlpattern collection as it does currently and make the urls
method return self.get_urls(), self.app_name, self.name
?.
This would simplfy the admin customization API and would keep the method consistent with its ModelAdmin
counterpart.
comment:73 by , 15 years ago
ramiro's suggestion sounds look a good one. And failing that then the auth UserAdmin needs to be updated for the new syntax.
comment:74 by , 15 years ago
Replying to ramiro:
Russell: (simple nitpick) If the user isn't supposed to modify the
app_name
andname
attibutes inside AdminSite.get_urls(), wouldn't it be better to leave it to return an urlpattern collection as it does currently and make theurls
method returnself.get_urls(), self.app_name, self.name
?.
This would simplfy the admin customization API and would keep the method consistent with its
ModelAdmin
counterpart.
Good idea, Ramiro. I've made this change on Github. I'm about to upload a new patch to reflect all the recent changes for those that aren't git-enabled.
by , 15 years ago
Attachment: | t10061-r11021.v3.diff added |
---|
Cleanup of some naming issues and other minor tweaks.
comment:75 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [11250]) Fixed #10061 -- Added namespacing for named URLs - most importantly, for the admin site, where the absence of this facility was causing problems. Thanks to the many people who contributed to and helped review this patch.
This change is backwards incompatible for anyone that is using the named URLs
introduced in [9739]. Any usage of the old admin_XXX names need to be modified
to use the new namespaced format; in many cases this will be as simple as a
search & replace for "admin_" -> "admin:". See the docs for more details on
the new URL names, and the namespace resolution strategy.
follow-up: 78 comment:76 by , 15 years ago
Did this fix make it into the django-1.1 release?
It's hard for me to tell by the dates.
comment:77 by , 15 years ago
Cc: | removed |
---|
comment:78 by , 15 years ago
Replying to mjs7231:
Did this fix make it into the django-1.1 release?
It's hard for me to tell by the dates.
Yes. A clue is it's got a 1.1 milestone and it's fixed. If it had been deferred from 1.1 the milestone would have been changed. If it didn't have a milestone or if you didn't trust it the sure way to check is to see here:
http://code.djangoproject.com/log/django/tags/releases/1.1
that 1.1 was tagged at r11366, which is after the r11250 changeset that fixed this ticket. Thus the fix is in the 1.1 release.
Confirmed. Only happens when one uses the
include(admin.site.urls)
form, so it's something introduced around r9739.