Opened 9 years ago

Closed 7 years ago

#6470 closed (fixed)

Admin urls should use urlpatterns

Reported by: jdetaeye Owned by: Alex Gaynor
Component: contrib.admin Version: master
Severity: Keywords: nfa-someday
Cc: schlaber@…, jay.wineinger@…, carl@…, ross@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

If your model has a string as key, there are some admin change pages that are not accessible.

If the primary key of your object ends in "add", "history" or "delete", you can't access the change form any more.

Culprit is the following code in admin\options.py, responsible for parsing the URL:

        if url is None:
            return self.changelist_view(request)
        elif url.endswith('add'):
            return self.add_view(request)
        elif url.endswith('history'):
            return self.history_view(request, unquote(url[:-8]))
        elif url.endswith('delete'):
            return self.delete_view(request, unquote(url[:-7]))
        else:
            return self.change_view(request, unquote(url))

A corrected version is:

        if url is None:
            return self.changelist_view(request)
        elif url == 'add':
            return self.add_view(request)
        elif url.endswith('/history'):
            return self.history_view(request, unquote(url[:-8]))
        elif url.endswith('/delete'):
            return self.delete_view(request, unquote(url[:-7]))
        else:
            return self.change_view(request, unquote(url))

This applies also to the current admin, but in the current only an object with a key equal to "add" will not be accessible (since the url processing is better).

Attachments (9)

nfa-options.diff (849 bytes) - added by tlpinney 9 years ago.
nfa-urls.diff (16.2 KB) - added by Alex Gaynor 8 years ago.
Initial work on implementing them using a property, not complete(although more or less functional), tests do not all pass
admin-urlpatterns-6470.diff (19.5 KB) - added by Michael Newman 8 years ago.
An update to the patch to apply cleanly to r8129
admin-urlpatterns-6470.2.diff (15.5 KB) - added by Alex Gaynor 8 years ago.
Latest version, full tests pass, and should be backwards compatible, updated docs
admin-urlpatterns.diff (15.9 KB) - added by Alex Gaynor 8 years ago.
up to date version of the patch
admin-urlpatterns.2.diff (15.9 KB) - added by Alex Gaynor 8 years ago.
small update to always use raw strings
admin-urlpatterns.3.diff (14.8 KB) - added by Alex Gaynor 8 years ago.
admin-urlpatterns.4.diff (17.3 KB) - added by Alex Gaynor 8 years ago.
admin-urlpatterns.5.diff (17.3 KB) - added by Alex Gaynor 8 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 9 years ago by Jacob

Triage Stage: UnreviewedAccepted

comment:2 Changed 9 years ago by Brian Rosner

Keywords: nfa-blocker added

Agreed. This should be fixed.

Changed 9 years ago by tlpinney

Attachment: nfa-options.diff added

comment:3 Changed 9 years ago by jkocherhans

Summary: Admin URLs are not adequately examinedAdmin urls should use urlpatterns

The fix for this ticket is waiting for some changes to the way url dispatch works. There isn't currently a way for the AdminSite class to supply urlpatterns.

comment:4 Changed 9 years ago by Alex Gaynor

Keywords: nfa-someday added; nfa-blocker removed

This won't(and doesn't need to) happen pre-merge.

Changed 8 years ago by Alex Gaynor

Attachment: nfa-urls.diff added

Initial work on implementing them using a property, not complete(although more or less functional), tests do not all pass

comment:5 Changed 8 years ago by Alex Gaynor

Version: newforms-adminSVN

Changed 8 years ago by Michael Newman

Attachment: admin-urlpatterns-6470.diff added

An update to the patch to apply cleanly to r8129

comment:6 Changed 8 years ago by Bernd

Cc: schlaber@… added

comment:7 Changed 8 years ago by anonymous

Cc: jay.wineinger@… added

comment:8 Changed 8 years ago by Carl Meyer

Cc: carl@… added

Changed 8 years ago by Alex Gaynor

Latest version, full tests pass, and should be backwards compatible, updated docs

comment:9 Changed 8 years ago by Alex Gaynor

Owner: changed from nobody to Alex Gaynor

comment:10 Changed 8 years ago by Alex Gaynor

Has patch: set

comment:11 Changed 8 years ago by Alex Gaynor

Just saw malcolm's comment on the 1.1 features list(about how multiple AdminSite instances will cause a probelm), and I'm 100% in agreement that that is an issue, however it's not an issue specific to my proposed method for handling this, you need to have a distinct name for each admin site's url, as such the admin site itself needs to have some concept of it's own name.

comment:12 Changed 8 years ago by Ross Poulton

Cc: ross@… added

Changed 8 years ago by Alex Gaynor

Attachment: admin-urlpatterns.diff added

up to date version of the patch

Changed 8 years ago by Alex Gaynor

Attachment: admin-urlpatterns.2.diff added

small update to always use raw strings

comment:13 Changed 8 years ago by Jacob

(In [9728]) In urlconfs, include() may now be used on an iterable of patterns instead of just a module string. Refs #6470 -- making the admin use a urlconf is much easier with this work done. Thanks, Alex Gaynor.

Changed 8 years ago by Alex Gaynor

Attachment: admin-urlpatterns.3.diff added

Changed 8 years ago by Alex Gaynor

Attachment: admin-urlpatterns.4.diff added

Changed 8 years ago by Alex Gaynor

Attachment: admin-urlpatterns.5.diff added

comment:14 Changed 8 years ago by Jacob

Resolution: fixed
Status: newclosed

(In [9739]) Fixed #6470: made the admin use a URL resolver.

This *is* backwards compatible, but admin.site.root() has been deprecated. The new style is ('^admin/', include(admin.site.urls)); users will need to update their code to take advantage of the new customizable admin URLs.

Thanks to Alex Gaynor.

comment:15 Changed 7 years ago by anonymous

Resolution: fixed
Status: closedreopened

There is a typo in contrib/admin/sites.py [9739], isn't it?

@@ -199,7 +199,7 @@
                 name='%sadmin_index' % self.name),
             url(r'^logout/$',
                 wrap(self.logout),
-                name='%sadmin_logout'),
+                name='%sadmin_logout' % self.name),
             url(r'^password_change/$',
                 wrap(self.password_change),
                 name='%sadmin_password_change' % self.name),

comment:16 Changed 7 years ago by dc

Resolution: fixed
Status: reopenedclosed

There is a separate ticket for this error.

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