Merge pull request #146 from rpkilby/secure-qs-default

Use more secure default for RelatedFilter.queryset

Author: Ryan P Kilby

README.rst

@@ -113,15 +113,15 @@ You can easily traverse multiple relationships when filtering by using ``Related
 
 
     class DepartmentFilter(filters.FilterSet):
-        manager = filters.RelatedFilter(ManagerFilter, name='manager')
+        manager = filters.RelatedFilter(ManagerFilter, name='manager', queryset=Manager.objects.all())
 
         class Meta:
             model = Department
             fields = {'name': ['exact', 'in', 'startswith']}
 
 
     class CompanyFilter(filters.FilterSet):
-        department = filters.RelatedFilter(DepartmentFilter, name='department')
+        department = filters.RelatedFilter(DepartmentFilter, name='department', queryset=Department.objects.all())
 
         class Meta:
             model = Company
@@ -140,13 +140,32 @@ Example filter calls:
     /api/companies?department__name=Accounting
     /api/companies?department__manager__name__startswith=Bob
 
+``queryset`` callables
+""""""""""""""""""""""
+
+Since ``RelatedFilter`` is a subclass of ``ModelChoiceFilter``, the ``queryset`` argument supports callable behavior.
+In the following example, the set of departments is restricted to those in the user's company.
+
+.. code-block:: python
+
+    def departments(request):
+        company = request.user.company
+        return company.department_set.all()
+
+    class EmployeeFilter(filters.FilterSet):
+        department = filters.RelatedFilter(filterset=DepartmentFilter, queryset=departments)
+        ...
+
+Recursive relationships
+"""""""""""""""""""""""
+
 Recursive relations are also supported. It may be necessary to specify the full module path.
 
 .. code-block:: python
 
     class PersonFilter(filters.FilterSet):
         name = filters.AllLookupsFilter(name='name')
-        best_friend = filters.RelatedFilter('people.views.PersonFilter', name='best_friend')
+        best_friend = filters.RelatedFilter('people.views.PersonFilter', name='best_friend', queryset=Person.objects.all())
 
         class Meta:
             model = Person
@@ -185,7 +204,7 @@ to all filter classes. It incorporates some of the implementation details of the
             return qs.filter(**{lookup_expr: isnull})
 
     class AuthorFilter(filters.FilterSet):
-        posts = filters.RelatedFilter('PostFilter')
+        posts = filters.RelatedFilter('PostFilter', queryset=Post.objects.all())
 
         class Meta:
             model = Author
@@ -285,15 +304,15 @@ You cannot combine ``AllLookupsFilter`` with ``RelatedFilter`` as the filter nam
 .. code-block:: python
 
     class ProductFilter(filters.FilterSet):
-        manufacturer = filters.RelatedFilter('ManufacturerFilter')
+        manufacturer = filters.RelatedFilter('ManufacturerFilter', queryset=Manufacturer.objects.all())
         manufacturer = filters.AllLookupsFilter()
 
 To work around this, you have the following options:
 
 .. code-block:: python
 
     class ProductFilter(filters.FilterSet):
-        manufacturer = filters.RelatedFilter('ManufacturerFilter')
+        manufacturer = filters.RelatedFilter('ManufacturerFilter', queryset=Manufacturer.objects.all())
 
         class Meta:
             model = Product
@@ -304,7 +323,7 @@ To work around this, you have the following options:
     # or
 
     class ProductFilter(filters.FilterSet):
-        manufacturer = filters.RelatedFilter('ManufacturerFilter', lookups='__all__')  # `lookups` also accepts a list
+        manufacturer = filters.RelatedFilter('ManufacturerFilter', queryset=Manufacturer.objects.all(), lookups='__all__')  # `lookups` also accepts a list
 
         class Meta:
             model = Product
@@ -326,15 +345,15 @@ and ``FilterSet``s from either package are compatible with the other's DRF backe
         ...
 
     class DRFFilter(rest_framework_filters.FilterSet):
-        vanilla = rest_framework_filters.RelatedFilter(filterset=VanillaFilter)
+        vanilla = rest_framework_filters.RelatedFilter(filterset=VanillaFilter, queryset=...)
 
 
     # invalid
     class DRFFilter(rest_framework_filters.FilterSet):
         ...
 
     class VanillaFilter(django_filters.FilterSet):
-        drf = rest_framework_filters.RelatedFilter(filterset=DRFFilter)
+        drf = rest_framework_filters.RelatedFilter(filterset=DRFFilter, queryset=...)
 
 
 Caveats & Limitations
@@ -380,6 +399,22 @@ The recommended solutions are to either:
     ?publish_date__range=2016-01-01,2016-02-01
 
 
+Migrating to 1.0
+----------------
+
+``RelatedFilter.queryset`` now required
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The related filterset's model is no longer used to provide the default value for ``RelatedFilter.queryset``. This
+change reduces the chance of unintentionally exposing data in the rendered filter forms. You must now explicitly
+provide the ``queryset`` argument, or override the ``get_queryset()`` method (see `queryset callables`_).
+
+
+``get_filters()`` renamed to ``expand_filters()``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+django-filter has add a ``get_filters()`` classmethod to it's API, so this method has been renamed.
+
 License
 -------
 Copyright (c) 2013-2015 Philip Neustrom <philipn@gmail.com>,

rest_framework_filters/filters.py

@@ -38,9 +38,9 @@ def fset(self, value):
 
     def get_queryset(self, request):
         queryset = super(RelatedFilter, self).get_queryset(request)
-        if queryset is not None:
-            return queryset
-        return self.filterset._meta.model._default_manager.all()
+        assert queryset is not None, \
+            "Expected `.get_queryset()` to return a `QuerySet`, but got `None`."
+        return queryset
 
 
 class AllLookupsFilter(AutoFilter):

tests/test_filtering.py

@@ -13,6 +13,7 @@
 )
 
 from .testapp.filters import (
+    UserFilter,
     PersonFilter,
     PostFilter,
     BlogPostFilter,
@@ -326,6 +327,28 @@ def test_related_filters_caching(self):
 
         self.assertEqual(len(filters), 1)
 
+    def test_relatedfilter_queryset_required(self):
+        # Use a secure default queryset. Previous behavior was to use the default model
+        # manager's `all()`, however this has the side effect of exposing related data.
+        # The default behavior should not expose information, which requires users to
+        # explicitly set the `queryset` argument.
+        class NoteFilter(FilterSet):
+            title = filters.CharFilter(name='title')
+            author = filters.RelatedFilter(UserFilter, name='author')
+
+            class Meta:
+                model = Note
+                fields = []
+
+        GET = {'author': User.objects.get(username='user2').pk}
+        f = NoteFilter(GET, queryset=Note.objects.all())
+
+        with self.assertRaises(AssertionError) as excinfo:
+            f.qs
+
+        msg = str(excinfo.exception)
+        self.assertEqual("Expected `.get_queryset()` to return a `QuerySet`, but got `None`.", msg)
+
 
 class MiscTests(TestCase):
     def test_multiwidget_incompatibility(self):

tests/test_performance.py

@@ -1,7 +1,7 @@
 from __future__ import print_function
 
 import argparse
-from timeit import timeit
+from timeit import repeat
 
 from django.test import TestCase, Client, override_settings
 
@@ -34,14 +34,16 @@ def test_sanity(self):
     def test_response_time(self):
         data = {'author__username': 'bob'}
 
-        df_time = timeit(lambda: self.client.get('/df-notes/', data), number=1000)
-        drf_time = timeit(lambda: self.client.get('/drf-notes/', data), number=1000)
+        df_time = min(repeat(lambda: self.client.get('/df-notes/', data), number=200, repeat=5))
+        drf_time = min(repeat(lambda: self.client.get('/drf-notes/', data), number=200, repeat=5))
+        diff = (drf_time - df_time) / df_time * 100.0
 
         if args.verbosity >= 2:
             print('\n' + '-' * 32)
             print('Response time performance')
             print('django-filter time:\t%.4fs' % df_time)
             print('drf-filters time:\t%.4fs' % drf_time)
+            print('performance diff:\t%+.2f%% ' % diff)
             print('-' * 32)
 
         self.assertTrue(drf_time < (df_time * self.threshold))

tests/testapp/filters.py

@@ -48,7 +48,7 @@ class Meta:
 
 class NoteFilterWithRelated(FilterSet):
     title = filters.CharFilter(name='title')
-    author = RelatedFilter(UserFilter, name='author')
+    author = RelatedFilter(UserFilter, name='author', queryset=User.objects.all())
 
     class Meta:
         model = Note
@@ -57,7 +57,7 @@ class Meta:
 
 class NoteFilterWithRelatedAll(FilterSet):
     title = filters.CharFilter(name='title')
-    author = RelatedFilter(UserFilterWithAll, name='author')
+    author = RelatedFilter(UserFilterWithAll, name='author', queryset=User.objects.all())
 
     class Meta:
         model = Note
@@ -66,7 +66,7 @@ class Meta:
 
 class NoteFilterWithRelatedAllDifferentFilterName(FilterSet):
     title = filters.CharFilter(name='title')
-    writer = RelatedFilter(UserFilterWithAll, name='author')
+    writer = RelatedFilter(UserFilterWithAll, name='author', queryset=User.objects.all())
 
     class Meta:
         model = Note
@@ -75,7 +75,7 @@ class Meta:
 
 class PostFilter(FilterSet):
     # Used for Related filter and Filter.method regression tests
-    note = RelatedFilter(NoteFilterWithRelatedAll, name='note')
+    note = RelatedFilter(NoteFilterWithRelatedAll, name='note', queryset=Note.objects.all())
     date_published = filters.AllLookupsFilter()
     is_published = filters.BooleanFilter(name='date_published', method='filter_is_published')
 
@@ -96,7 +96,7 @@ def filter_is_published(self, qs, name, value):
 
 class CoverFilterWithRelatedMethodFilter(FilterSet):
     comment = filters.CharFilter(name='comment')
-    post = RelatedFilter(PostFilter, name='post')
+    post = RelatedFilter(PostFilter, name='post', queryset=Post.objects.all())
 
     class Meta:
         model = Cover
@@ -105,7 +105,7 @@ class Meta:
 
 class CoverFilterWithRelated(FilterSet):
     comment = filters.CharFilter(name='comment')
-    post = RelatedFilter(PostFilter, name='post')
+    post = RelatedFilter(PostFilter, name='post', queryset=Post.objects.all())
 
     class Meta:
         model = Cover
@@ -114,7 +114,7 @@ class Meta:
 
 class PageFilterWithRelated(FilterSet):
     title = filters.CharFilter(name='title')
-    previous_page = RelatedFilter(PostFilter, name='previous_page')
+    previous_page = RelatedFilter(PostFilter, name='previous_page', queryset=Post.objects.all())
 
     class Meta:
         model = Page
@@ -131,7 +131,7 @@ class Meta:
 
 class BlogPostFilter(FilterSet):
     title = filters.CharFilter(name='title')
-    tags = RelatedFilter(TagFilter, name='tags')
+    tags = RelatedFilter(TagFilter, name='tags', queryset=Tag.objects.all())
 
     class Meta:
         model = BlogPost
@@ -186,7 +186,7 @@ class Meta:
 
 class PersonFilter(FilterSet):
     name = AllLookupsFilter(name='name')
-    best_friend = RelatedFilter('tests.testapp.filters.PersonFilter', name='best_friend')
+    best_friend = RelatedFilter('tests.testapp.filters.PersonFilter', name='best_friend', queryset=Person.objects.all())
 
     class Meta:
         model = Person