Code

Opened 6 years ago

Closed 5 years ago

#6470 closed (fixed)

Admin urls should use urlpatterns

Reported by: jdetaeye Owned by: Alex
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 6 years ago.
nfa-urls.diff (16.2 KB) - added by Alex 6 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 Mnewman 6 years ago.
An update to the patch to apply cleanly to r8129
admin-urlpatterns-6470.2.diff (15.5 KB) - added by Alex 6 years ago.
Latest version, full tests pass, and should be backwards compatible, updated docs
admin-urlpatterns.diff (15.9 KB) - added by Alex 5 years ago.
up to date version of the patch
admin-urlpatterns.2.diff (15.9 KB) - added by Alex 5 years ago.
small update to always use raw strings
admin-urlpatterns.3.diff (14.8 KB) - added by Alex 5 years ago.
admin-urlpatterns.4.diff (17.3 KB) - added by Alex 5 years ago.
admin-urlpatterns.5.diff (17.3 KB) - added by Alex 5 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 6 years ago by jacob

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 6 years ago by brosner

  • Keywords nfa-blocker added

Agreed. This should be fixed.

Changed 6 years ago by tlpinney

comment:3 Changed 6 years ago by jkocherhans

  • Summary changed from Admin URLs are not adequately examined to Admin 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 6 years ago by Alex

  • Keywords nfa-someday added; nfa-blocker removed

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

Changed 6 years ago by Alex

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

comment:5 Changed 6 years ago by Alex

  • Version changed from newforms-admin to SVN

Changed 6 years ago by Mnewman

An update to the patch to apply cleanly to r8129

comment:6 Changed 6 years ago by Bernd

  • Cc schlaber@… added

comment:7 Changed 6 years ago by anonymous

  • Cc jay.wineinger@… added

comment:8 Changed 6 years ago by carljm

  • Cc carl@… added

Changed 6 years ago by Alex

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

comment:9 Changed 6 years ago by Alex

  • Owner changed from nobody to Alex

comment:10 Changed 6 years ago by Alex

  • Has patch set

comment:11 Changed 5 years ago by Alex

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 5 years ago by rossp

  • Cc ross@… added

Changed 5 years ago by Alex

up to date version of the patch

Changed 5 years ago by Alex

small update to always use raw strings

comment:13 Changed 5 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 5 years ago by Alex

Changed 5 years ago by Alex

Changed 5 years ago by Alex

comment:14 Changed 5 years ago by jacob

  • Resolution set to fixed
  • Status changed from new to closed

(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 5 years ago by anonymous

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 5 years ago by dc

  • Resolution set to fixed
  • Status changed from reopened to closed

There is a separate ticket for this error.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.