Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using default queryset is insecure when filtering across relationships #100

Open
rpkilby opened this issue Jul 31, 2016 · 4 comments
Open
Milestone

Comments

@rpkilby
Copy link
Collaborator

rpkilby commented Jul 31, 2016

Filtering across relationships currently uses the default queryset. For example:

class ArticleFilter(FilterSet):
    is_published = filers.BooleanFilter()
    body = filter.CharFilter()

class AuthorFilter(FilterSet):
    article = filters.RelatedFilter(ArticleFilter)

class AuthorViewSet(viewsets.ModelViewSet):
    queryset = models.Author.objects.all()
    filter_class = AuthorFilter

class PostViewSet(viewsets.ModelViewSet):
    queryset = models.Post.objects.all()
    filter_class = PostFilter

    def get_queryset(self):
        qs = super(PostViewSet, self).get_queryset()
        user = self.request.user

        # get user's posts and published posts
       user_posts = qs.filter(author__user=user)
       published_posts = qs.filter(is_published=True)

       return user_posts | published_posts

While you're not able to view unpublished posts of other authors, you are able to filter across the relationship and derive that an author may be preparing an article about some topic.

eg:

/api/authors?post__is_published=false&post__body__contains=juicy%20story%20details
@rpkilby rpkilby modified the milestone: v1.0.0 Aug 2, 2016
@rpkilby rpkilby mentioned this issue Aug 2, 2016
@rpkilby
Copy link
Collaborator Author

rpkilby commented Aug 12, 2016

This also leaks information in the rendered form, as the dropdown for the ModelChoiceFilter will contain the entire queryset. eg, a related product filter might display your entire customer list.

@rpkilby
Copy link
Collaborator Author

rpkilby commented Jul 14, 2018

It should be documented that a RelatedFilter.queryset should be consistent with the logic applied in ViewSet.get_queryset(). As explained above, an inconsitency would have the potential to leak data.

My recommendation would be to write queryset methods that provides the set of objects that a user should have read access to, then use that queryset method where necessary. e.g.,

# Get authors/books for which users have read permissions
class AuthorQuerySet(models.QuerySet):
    def for_user(self, user):
        return self.filter(something=user)

class BookQuerySet(models.QuerySet):
    def for_user(self, user):
        return self.filter(something=user)

# filters
class AuthorFilterSet:
    ...

class BookFilterSet:
    author = RelatedFilter(queryset=Author.objects.for_request)
    ...

# views
class AuthorViewSet:
    filterset_class = AuthorFilterSet
    def get_queryset(self):
        return Author.objects.for_request(self.request)

class BookViewSet:
    filterset_class = BookFilterSet
    def get_queryset(self):
        return Book.objects.for_request(self.request)

@sassanh
Copy link

sassanh commented Oct 25, 2019

@rpkilby when using related filter I can pass a queryset to it and it works. Like this for example:

    house__furniture__electrical = RelatedFilter(SomeFilter, queryset=some_custom_queryset)

So I think this issue is resolved and can be closed, what do you think?

@rpkilby
Copy link
Collaborator Author

rpkilby commented Oct 25, 2019

Hi @sassanh. Thanks - this is more a reminder that the docs need to be updated to better inform the user on how to use RelatedFilter.queryset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants