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

Accessing sub-resources of inaccessible parent resources doesn't work as expected #216

Open
samul-1 opened this issue Mar 11, 2021 · 7 comments

Comments

@samul-1
Copy link

samul-1 commented Mar 11, 2021

Summary: I have an Exercise model, and a Submission model. Submissions represent solutions submitted to an exercise, and are henche a sub-resource of exercises. I have the following path: /exercises/{:id}/submissions.

The exercise viewset has a filter such that non-staff users can only access certain exercises: accessing an exercise outside of that list results in a 404 error. However, it appears that if a user tries to access the /submissions of an exercise they can't access, instead of getting a 404 there too, they can normally access those submissions and even POST to the url.

I believe a reasonable behavior would be to inherit the filtering done on the parent resource and use that on the sub-resource. I acknowledge that there might already be a way to do this, and I've just missed it. Any help or input is appreciated.

Code for the viewsets:

class ExerciseViewSet(viewsets.ModelViewSet):
    """
    A viewset for viewing, creating, and editing exercises

    Only staff members can create or update exercises
    """

    serializer_class = ExerciseSerializer
    queryset = Exercise.objects.all()

    # only allow teachers to create or update exercises
    permission_classes = [IsTeacherOrReadOnly]

    # only allow regular users to see the exercise that's been assigned to them
    filter_backends = [filters.TeacherOrAssignedOnly]


class SubmissionViewSet(viewsets.ModelViewSet):
    """
    A viewset for listing, retrieving, and creating submissions to a specific exercise, and
    turning in eligible submissions.

    POST requests are limited to once every 30 seconds.

    Staff members can access submissions by all users to a specific exercise, whereas
    normal users can only access theirs
    """

    serializer_class = SubmissionSerializer
    filter_backends = [filters.TeacherOrOwnedOnly]
    queryset = Submission.objects.all()

    def get_throttles(self):
        if self.request.method.lower() == "post":
            # limit POST request rate
            return [throttles.UserSubmissionThrottle()]

        return super(SubmissionViewSet, self).get_throttles()

    def get_queryset(self):
        queryset = super(SubmissionViewSet, self).get_queryset()

        exercise_id = self.kwargs["exercise_pk"]
        user_id = self.request.query_params.get("user_id", None)

        # filter submissions for given exercise
        if exercise_id is not None:
            exercise = get_object_or_404(Exercise, pk=exercise_id)
            queryset = queryset.filter(exercise=exercise)

        # filter submissions for given user
        if user_id is not None:
            user = get_object_or_404(User, pk=user_id)
            queryset = queryset.filter(user=user)

        return queryset

Full repo: https://github.com/samul-1/js-exercise-platform/

@Mogost
Copy link

Mogost commented Jul 28, 2022

Also caught this

@samul-1
Copy link
Author

samul-1 commented Jan 11, 2023

No updates on this?

@alanjds
Copy link
Owner

alanjds commented Jan 12, 2023

The Nested* routers do not have a way to filter the access. They just to "mount" the URLs over the parent resources.

Filtering the child based on the parent access should be done on the view level by the user. To ease this common need, there is a rest_framework_nested.viewsets.NestedViewSetMixing, that have a get_queryset() implementation (https://github.com/alanjds/drf-nested-routers/blob/master/rest_framework_nested/viewsets.py#L42) that filters the child based on the parent access, as you expect.

Yet it needs some configuration. You will need to take a look on the code and/or on the README examples about how to set the parent_lookup_kwargs attribute on the child view. Or you can just overload the get_queryset() yourself to do it.

@samul-1
Copy link
Author

samul-1 commented Jan 12, 2023

Thank you @alanjds. Taking a look at the code of that mixin, it appears that the filtering looks something like: qs.filter(parent_pk_fieldname=parent_pk_value). However, if the parent with supplied pk doesn't exist, this simply returns an empty queryset.

This makes sense from an ORM standpoint, as that's what the query from the database returns; however, you can easily imagine for an API a different behavior would be desired.

On top of non-existing parents, one could want to limit access to children of parent objects that the user is not authorized to see.

What I can imagine is that, for a list action to get all the children of some parent object, the available parent objects should be all and only those that can be accessed in detail mode from the viewset of the parent resource.

I'm thinking of a way to produce something like a spurious detail action request to the viewset of the parent resource, see if the request succeeds (i.e. if the view successfully retrieves an object), and use that information to determine what actions are available on the child resource for the children of that object.

Another approach could be to use the parent viewset to generate the queryset for the parent resource (once again, mimicking a detail request) and add a filter to the qs for the child viewset: qs.filter(parent_pk_fieldname=parent_pk_value, parent_pk_in=qs_returned_from_parent_viewset).

Would this be outside the scope of this project, or could it be something worth exploring? I believe this would be a common requirement in projects that have nested resources.

@alanjds
Copy link
Owner

alanjds commented Jan 12, 2023

Interesting.

I think this to be in the scope of this project. Specially cause this is a common need/request after the person got the resource nested.

There were some requests in the past asking for applying the parent filtering, honor per-object permissions, and so. And I ended up creating this mixin in the hope to cover the 80% times where the simplest thing is enough. Then saying "go custom if you want something more elaborate"

However, I liked the idea of querying the parent and using that as the decisor. This is generic enough to accommodate any restriction scheme the person could had imposed to the parent. Will lead to some extra DB or CPU burden, but at least will fix 80% of this 20% cases remaining I guess... (16% of all cases? letting only 4% of the universe uncovered! 😬 How satisfying is to use fake statistics.)

@alanjds
Copy link
Owner

alanjds commented Jan 12, 2023

Now, about filtering the full list of the parent and use that full list over a SELECT IN clause over the child, there I see more burden than benefits. Overfetching from DB and possibly making a loooong (and costly) SQL filter. Yet this is just my current opinion.

@Mogost
Copy link

Mogost commented Jan 12, 2023

However, I liked the idea of querying the parent and using that as the decisor. This is generic enough to accommodate any restriction scheme the person could had imposed to the parent. Will lead to some extra DB or CPU burden

I also like this idea. I am ready to pay this bill with CPU and DB. But I want to know that permission was checked without code duplication.

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

3 participants