[Django] #23482: SingleObjectMixin.get_object() should allow lookup by BOTH pk and slug
Django
-------------------------------+--------------------
Reporter: jdufresne | Owner: nobody
Type: New feature | Status: new
Component: Generic views | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
Using generic views such as DetailView.
Problem:
I do not want to use the object PK as the only differentiating URL
component, as it would allow users to brute force guess all URLs and gain
knowledge of all objects. In my application, users have permission to view
individual objects, but letting users know a complete list of all objects
is undesirable.
First thought, use a non-sequential slug. The slug could be based on the
name of the object. However, the obvious source of the slug is not
necessarily unique. Two objects can have identical names and this is OK
for the object. I'd prefer to avoid additional overhead of calculating a
unique slug.
Next thought, append the PK to the name to generate the slug: <name>-<pk>.
This works as a unique slug, but has the drawback that a newly created
object cannot create a slug until *after* the save when the
`auto_increment` kicks in.
Next thought, keep the slug as non-unique, but use both the slug and PK in
the URL. So the slug would remain the name, and the URL would contain:
`/<slug>-<pk>/`. Then look up the object by *both* the slug and the PK.
This solves:
* URLs aren't brute force guessable without first knowing the object you
want to lookup
* It is OK that the slug is not unique, the URL is.
* URLs are prettier than a simple PK as the slug is more friendly to
humans
* The slug can be generated before save without a database hit
However, the default implementation of `SingleObjectMixin.get_object()`
uses *either* the PK or the slug, in that order, but not both. I am
suggesting that it be modified to use both fields if they are both
available. This would be as simple as changing the `elif slug is not
None:` from `elif` to an `if` (with some minor cleanup for error checking
and tests).
If this change is agreeable, I would be happy to create a pull request
that solves this problem.
(I realize I can implement this in my own views, and will if I need to,
but I thought this would be nice built-in.)
--
Ticket URL: <https://code.djangoproject.com/ticket/23482>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jdufresne | Owner: nobody
Type: New feature | Status: new
Component: Generic views | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by freakboy3742):
* needs_better_patch: => 0
* stage: Unreviewed => Accepted
* needs_tests: => 0
* needs_docs: => 0
Comment:
The approach you've described is a common way to combat
[https://www.owasp.org/index.php/Top_10_2013-A4-Insecure_Direct_Object_References
OWASP2013:A4 "Insecure Object References"], so it makes sense to me that
we should support it at the API level.
--
Ticket URL: <https://code.djangoproject.com/ticket/23482#comment:1>
Django
Reporter: jdufresne | Owner: nobody
Type: New feature | Status: new
Component: Generic views | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jdufresne):
https://github.com/django/django/pull/3216
Created pull request.
--
Ticket URL: <https://code.djangoproject.com/ticket/23482#comment:2>
Django
Reporter: jdufresne | Owner: nobody
Type: New feature | Status: new
Component: Generic views | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by claudep):
* needs_docs: 0 => 1
* has_patch: 0 => 1
Comment:
Needs an update here, at least:
https://docs.djangoproject.com/en/dev/ref/class-based-views/mixins-single-
object/#django.views.generic.detail.SingleObjectMixin.get_object
--
Ticket URL: <https://code.djangoproject.com/ticket/23482#comment:3>
Django
Reporter: jdufresne | Owner: nobody
Type: New feature | Status: new
Component: Generic views | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by jdufresne):
I have updated the documentation. I do not feel I am a great technical
writer, so please feel free to suggest any changes if anything is lacking
or unclear.
Right now, for the sake of review and tracking progress, my changes span
multiple commits. Once review is complete, I'll squash the commits to one
if preferred.
--
Ticket URL: <https://code.djangoproject.com/ticket/23482#comment:4>
Django
Reporter: jdufresne | Owner: nobody
Type: New feature | Status: new
Component: Generic views | Version: master
Keywords: | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timgraham):
* needs_docs: 1 => 0
* stage: Accepted => Ready for checkin
Comment:
I will do a final review tomorrow.
--
Ticket URL: <https://code.djangoproject.com/ticket/23482#comment:5>
Django
Reporter: jdufresne | Owner: nobody
Type: New feature | Status: closed
Component: Generic views | Version: master
Keywords: | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"8c581ff39475c1b3b25d60945cc1c73a7f8eb1be"]:
{{{
#!CommitTicketReference repository=""
revision="8c581ff39475c1b3b25d60945cc1c73a7f8eb1be"
Fixed #23482 -- Added SingleObjectMixin.query_pk_and_slug
Enabling the attribute causes get_object() to perform its lookup
using both the primary key and the slug.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23482#comment:6>