The use of eval()
here is less than ideal as it allows you to call the URL delete/User/3/
, for instance, and it will work against all odds. Worse, you can call the URL delete/delete/9
where eval will resolve delete
to this view function and lead to all kind of weird behavior.
So the first step is to restrict which symbols can be found from there. The easiest way is to retrieve the attribute directly from your models:
from . import models
...
def delete(request, type_name, object_id):
try:
cls = getattr(models, type_name)
except AttributeError:
raise Http404
get_object_or_404(cls, id=object_id).delete()
redirect(dashboard)
But this is still too large as anything that you imported in models
can still be retrieved. So you're better off using a base class in your models.py
that only your deletable models will inherit from:
class Deletable:
def can_delete(self, user):
return user.is_active and user.is_staff
class OneOfYourCustomModel(Deletable, models.Model):
# whatever
And check in your view that whatever you get is Deletable:
def delete(request, type_name, object_id):
try:
cls = getattr(models, type_name)
except AttributeError:
raise Http404
if inspect.isclass(cls) and issubclass(cls, models.Deletable):
get_object_or_404(cls, id=object_id).delete()
else:
raise Http404
redirect(dashboard)
You'll also note that it also lead to easy customization of which user can delete what: you just need to override one method and add proper logic (eg raising 403 forbidden) in your view. This can be more useful than the @staff_member_required
in case your models can be associated to (owned by) a particular user:
class MyOwnedModel(Deletable, models.Model):
owner = models.ForeignKey(django.contrib.auth.models.User, …)
def can_delete(self, user):
return user == self.owner or super().can_delete(user)
But of course, if only the staff can delete things, the decorator is plenty sufficient.
Lastly, it would be good to separate the "retrieve the actual model" logic from the "delete the particular instance of that model" one. The latter being the responsibility of the view, but the former would better fit as a custom URL converter.
The main advantage being that you don't need to handle the 404s for unknown models in your view, django will take care of that itself if your converter ever raises a ValueError
. Going this route, you can have a converters.py
:
from inspect import isclass
from django.urls.converters import StringConverter
from . import models
class DeletableConverter(StringConverter):
def to_python(self, value):
try:
cls = getattr(models, value)
except AttributeError:
raise ValueError
if not isclass(cls) or not issubclass(cls, models.Deletable):
raise ValueError
return cls
def to_url(self, value):
return value.__name__
Usage in your urls.py
:
from django.urls import path, register_converter
from . converters import DeletableConverter
register_converter(DeletableConverter, 'deletable')
urlpatterns = [
…,
path('delete/<deletable:model>/<int:object_id>', views.delete),
…,
]
And in your view:
def delete(request, model, object_id):
instance = get_object_or_404(model, id=object_id)
if instance.can_delete(request.user):
# or a simple @staff_member_required decorator
instance.delete()
redirect(dashboard)
urls.py
and added an example above. \$\endgroup\$