Ircam-WAM / mezzanine-organization

Organization module for the Mezzo CMS
GNU Affero General Public License v3.0
2 stars 0 forks source link

SlugMixin doesn't respect self.queryset (or self.get_queryset) #12

Closed raphaelyancey closed 5 years ago

raphaelyancey commented 5 years ago

https://github.com/Ircam-Web/mezzanine-organization/blob/3565c8c72b6a2ea8d363f9b4e217f36838a32ec4/organization/core/views.py#L54-L58

If the inheriting class has a custom queryset via self.queryset = Model.objects.filter(....) or def get_queryset(self): ..., the SlugMixin will ignore it and return an object even if it shouldn't.

This is a security issue that can be used to view unpublished (published() manager) objects for example:

class ForumArticleDetailView(SlugMixin, DetailView):

    model = ForumArticle
    template_name='magazine/article/article_detail.html'
    context_object_name = 'article'
    queryset = ForumArticle.objects.published()  # It will be ignored because of SlugMixin
raphaelyancey commented 5 years ago

On a side note, what is the use of SlugMixin really as Django behaves this way by default ? https://docs.djangoproject.com/fr/1.10/ref/class-based-views/mixins-single-object/#django.views.generic.detail.SingleObjectMixin.get_object

get_object() looks for a pk_url_kwarg argument in the arguments to the view; if this argument is found, this method performs a primary-key based lookup using that value. If this argument is not found, it looks for a slug_url_kwarg argument, and performs a slug lookup using the slug_field.

If the object slug field is slug and the URL parameter is also slug it works out of the box.

yomguy commented 5 years ago

You are right @raphaelyancey, thanks for the report and the fix.

Yes django could deal with it out of the box, but the fact is that Mezzanine uses its own slug attributes linked to some customizable methods like this one : https://github.com/stephenmcd/mezzanine/blob/master/mezzanine/core/models.py#L111

So, not sure we should switch to the native behavior without having side effects in our case.