incorrect logout link in admin

[Tim Miller]

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.

[Malcolm Tredinnick]

Confirmed. Only happens when one uses the include(admin.site.urls) form, so it's something introduced around r9739.

[Alex Gaynor]

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.

[Alex Gaynor]

attached patch should solve the issue

[Alex Gaynor]

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

[Alex Gaynor]

added tests

[Alex Gaynor]

fixed issues for admin and auth, also cleaned up the URL tag, it shouldn't alter it's own state if not necessary

[jleingang]

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

[Alex Gaynor]

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.

[Batiste Bieler]

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.

[Thomas Schreiber]

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

[Thomas Schreiber]

sorry, didn't mean to close it

[Alex Gaynor]

Oh hey, just uploaded the same patch again, that was smart.

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

[Johan]

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.

[Johan]

I've attempted my first try at a patch. Here its is. Its much simpler than the other patches. Am i missing something ?

[Alex Gaynor]

That's a) backwards incompatible, and b) doesn't work with multiple admin sites.

[Alex Gaynor]

updated for merge conflicts

[Alex Gaynor]

a few extra lines of documentation

[Matthew Jacobi]

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.

[Carl Meyer]

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.

[dc]

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?

[dc]

Also change in core/urlresolvers.py is backwards-incompatible as someone can use ':' in view names.

[Malcolm Tredinnick]

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

[dc]

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.

[Malcolm Tredinnick]

Yes, you are mistaken. That's not why we're using namespaces. Unsurprisingly, it will be documented.

[dc]

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?

[Malcolm Tredinnick]

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

[dc]

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.

[Alex Gaynor]

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

[sorl]

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?

[sorl]

sorry I didn't reed up properly, discard my suggestion, just use it if you want something working now.

[leemarrett]

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

[mmara]

I can confirm the problem still exists with rev10980

[anonymous]

people, you don't want to release 1.1 with this embarassing bug, really!

[David Larlet]

No, and that's why it's tagged with 1.1 milestone.

[Alex Gaynor]

Probably just a whitespace change, but I can't remember if I've had to resolve any merge conflicts

[Collin Anderson]

Sorry for the noise.

[anonymous]

Is anyone working on this?

[Russell Keith-Magee]

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.

[Juan]

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.

[Russell Keith-Magee]

First attempt at a complete namespace lookup fix for redeployed applications

[Russell Keith-Magee]

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.

[Rob Hudson <treborhudson@…>]

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?

[Rob Hudson <treborhudson@…>]

patch on t10061-r11201

[Alex Gaynor]

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.

[Russell Keith-Magee]

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.

[Russell Keith-Magee]

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.

[Russell Keith-Magee]

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.

[Russell Keith-Magee]

Updated patch including suggestions from Alex and Rob

[Russell Keith-Magee]

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.

[Russell Keith-Magee]

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.

[Russell Keith-Magee]

Third pass - now passing full test suite.

[dc]

Russell, may i suggest using isinstance() for type checking?
Better replace

type(view) == tuple

with

isinstance(view, tuple)

Not critical but more pythonic.

[anonymous]

Replying to dc:

Russell, may i suggest using isinstance() for type checking?
Better replace

type(view) == tuple

with

isinstance(view, tuple)

Not critical but more pythonic.

And more PEP8

[Alex Gaynor]

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

[Russell Keith-Magee]

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

[Russell Keith-Magee]

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

[Russell Keith-Magee]

For those interested, I'm pushing my modifications into my public git repository: ​http://github.com/freakboy3742/django/tree/t10061-rkm

[Ramiro Morales]

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.

[Alex Gaynor]

ramiro's suggestion sounds look a good one. And failing that then the auth UserAdmin needs to be updated for the new syntax.

[Russell Keith-Magee]

Replying to ramiro:

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.

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.

[Russell Keith-Magee]

Cleanup of some naming issues and other minor tweaks.

[Russell Keith-Magee]

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

[mjs7231]

Did this fix make it into the django-1.1 release?
It's hard for me to tell by the dates.

[Karen Tracey]

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.

[Antti Kaihola]

For cross reference: r11250 broke patch 2​ for #11163 (wrong ForeignKeyRawIdWidget link in admin change list view). I haven't figured out what the exact reason is, but once I do, I'll prepare a new patch for #11163. Any ideas?

[Jacob]

Milestone 1.1 deleted