Missing permission checks in RelatedMixin views

[lcary]

No object level or model level permissions appear to be checked for related models in RelatedMixin views.

Expected behavior: The related fields should be empty in a response object if the user doesn't have view permission to the related field. Alternatively, if a response must render the related object, a permissions error should be raised.

Observed behavior: Currently, if the user has permission for a model but not related models (e.g. via a foreign key relation), they are still able to view the related object if a RelatedMixin view's serializer has a ResourceRelatedField. This seems to merit at least some documentation warning users about the potential for leaking data, but preferably this issue could be fixed by enabling permissions checking.

Reproducing the issue requires a project that checks that a user has read (or "view") permissions for models they try to access (e.g. using extended versions of DjangoObjectPermissions and DjangoModelPermissions classes mentioned in the django-rest-framework permissions documentation). In such a project, for any RelatedMixin view that uses a serializer with a ResourceRelatedField, grant the user permission to the serializer's model object, but do not grant the user access to the related model. If the user requests the object, they will still be able to access the related field. I'm happy to add a test case for this issue.

Perhaps object level or even model level permissions checking is out of scope for the django-rest-framework-json-api project, but it seems like both should be supported. After all, django-rest-framework supports and advertises both via the DjangoObjectPermissions and DjangoModelPermissions classes. In a project I'm working on, we use django-guardian with a similar class to check object-level permissions. Perhaps I can override RelatedMixin.get_related_instance() in the meantime as a stopgap solution, but a solution via configuration flag or a default in this library would be preferable.

Best,
Luc

[sliverc]

Fair point. I guess self.check_object_permissions(self.request, obj) should be run in get_related_instance before it returns instance. I guess this would solve your issue?

I think a hint in the documentation should note that permission_classes on views used with relation links should be aware of different object types. This should already be the case with DjangoObjectPermissions and DjangoModelPermissions classes.

A PR would be welcome.

[n2ygk]

@sliverc I'll take this and add to my open PR (#492) for now as this is touching the same RelatedMixin code (and @lcary and I are coworkers;-). Once we figure it out we can split out into separate PRs if warranted. Does that make sense? Handing off to @lcary to do a separate PR.

[n2ygk]

I believe the following areas need to be tested for appropriate permissions. Example URLs below based on the drf_example course and term data.

• relationships data in the top-level query
  http://127.0.0.1:8000/courses/01ca197f-c00c-4f24-a743-091b62f1d500

   "data": {
       "type": "course",
       "id": "01ca197f-c00c-4f24-a743-091b62f1d500",
       "attributes": {
             ...
       },
       "relationships": {
           "terms": {
               "meta": {
                   "count": 2
               },
               "data": [
                   {
                       "type": "term",
                       "id": "f9aa1a51-bf3b-45cf-b1cc-34ce47ca9913"
                   },
                   {
                       "type": "term",
                       "id": "01163a94-fc8f-47fe-bb4a-5407ad1a35fe"
                   }
               ],
               "links": {
                   "self": "http://127.0.0.1:8000/courses/01ca197f-c00c-4f24-a743-091b62f1d500/relationships/terms",
                   "related": "http://127.0.0.1:8000/courses/01ca197f-c00c-4f24-a743-091b62f1d500/terms/"
               }
           }
       },
       ...

• relationships view link from above
  http://127.0.0.1:8000/courses/01ca197f-c00c-4f24-a743-091b62f1d500/relationships/terms
• included
  http://127.0.0.1:8000/courses/01ca197f-c00c-4f24-a743-091b62f1d500?include=terms
• related data
  data[].relationships..links.related URL should have same result as the equivalent top-level view with a filter[parent.id]=<parent.id>
  http://127.0.0.1:8000/courses/01ca197f-c00c-4f24-a743-091b62f1d500/terms/ same as
  http://127.0.0.1:8000/terms?filter[course.id]=01ca197f-c00c-4f24-a743-091b62f1d500 (assuming filters are defined -- which is currently not the case in the example app but can be added.)
• relationships, include for the relationships and related links?

Am I missing anything?

What the correct "denial" response should be is also up in the air: return nothing or raise an exception.

Raising an exception seems harsh; I can imagine a resource that has several relationships and it's still useful to know about those that are permitted while not seeing those that are not permitted. I'll go over to the JSON:API forum and see if there are any best practices.

[sliverc]

Let's not mix up different issues in one PR but create one PR for each of those two issues. Merge conflicts can always be solved later one but splitting up a PR is much harder.

I do not think this is a matter of JSON API spec but of HTTP standard hence when someone doesn't have the permission to raise a 403 error makes most sense then rendering partial of the results (which might not be obvious why by API user).

When using self.check_object_permissions(self.request, obj) this is automatically the case.

Saying this let's not validate included_serializers resp. include param as one end point should have a permission level for the whole object it returns and this wouldn't be the case when each include has its own permission level. For included_serializers I would rather prefer that this is documented that included relationships need to have the same permission as parent.

[n2ygk]

OK about mixing PRs. I can certainly do it separately.

included resources is not the only case of disclosing data from an underlying Model that the request.user may not have access to. It is also present as the type and id in the relationships/<rel> data that is in the main response. While this is a limited disclosure, it is still disclosing information:

"data": [
                 {
                     "type": "term",
                     "id": "f9aa1a51-bf3b-45cf-b1cc-34ce47ca9913"
                 },
                 {
                     "type": "term",
                     "id": "01163a94-fc8f-47fe-bb4a-5407ad1a35fe"
                 }
             ],

In fact, disclosing that there is/are any relationships present itself is a problem if the request.user doesn't have permission to the related Model.

And you certainly don't want the top-level GET to raise an exception because of unavailable ResourceRelatedFields.

In a more complex example (e.g. example.blog entry, which has 9 relationships) you might want the user to be able to see 5 of them but not the other 4. Or for a toMany relationship, return some but not all items (e.g. when using row-level permissions).

The toMany partial response was discussed in JSON:API's discussion group but not really with a satisfactory result. One approach -- JSONAPI extension for Drupal -- gives an overall 200 response but adds meta.errors for individual members of relationships that are not returned. But that doesn't even cover the included and relationships portions.

Then there's the other case of field-level permissions. This seems to just be a feature one would implement in the Model or Serializer.

I'll do some more digging in the JSON:API forum and see if this is covered and will post a question if not.

Certainly documenting the status quo that related Models are subject to the parent's permissions can be documented now.

Perhaps we make an optional Mixin so that those who want more granular permissions for related fields can get the functionality.

[lcary]

@sliverc regarding your comment here:

Fair point. I guess self.check_object_permissions(self.request, obj) should be run in get_related_instance before it returns instance. I guess this would solve your issue?

Yes, I think calling check_object_permissions() in get_related_instance() is the correct approach, and it makes sense to add documentation about object permissions for views that expose related objects.

For example, on line 167:

            related_instance = field.get_attribute(parent_obj)
            self.check_object_permissions(self.request, related_instance)
            return related_instance

And on line 170:

                related_instance = getattr(parent_obj, field_name)
                self.check_object_permissions(self.request, related_instance)
                return related_instance

However, @n2ygk also raises a good point here:

In a more complex example (e.g. example.blog entry, which has 9 relationships) you might want the user to be able to see 5 of them but not the other 4. Or for a toMany relationship, return some but not all items (e.g. when using row-level permissions).

In the more complex example of a toMany relationship - where we want to filter a parent object's related field set based on object-level permissions - is there a convenient way for the library to defer to the view's filter_class?

Spitballing:

1. Check if the related object serializer has many=True set,
2. Use the filter_class for the view by default to filter the queryset for the related instances (note: we might need a class attribute for overriding this at the related field level),
3. Apply the filter to the related object set, and only show those results.

Via step 2 above, the filtering of the related objects is done by default via the view's filter. On our views, we use a DjangoObjectPermissionsFilter by default, so this would have the desired behavior of only rendering related objects that the user has permission to, similar to how a list view would function for that related object.

Finally, I think included_serializers validation is out of scope, and agree that it should be a separate PR (and potentially a separate issue). Otherwise, a PR to fix this would be more complex/greedy than the issue merits.

[n2ygk]

https://discuss.jsonapi.org/t/hide-unfetchable-relationships-return-404-other/1405

[sliverc]

@n2ygk @lcary
I see the challenge you have but I think this issue is not in DJA directly but rather in DRF. When you e.g. use a PrimaryKeyRelatedField relationship it doesn't do any permission checks either.
Also DjangoObjectPermissionFilter states that this is only a view permission and not for relationships as well.

Hence I suggest the following:

1. Open a PR with check_object_permissions patch (this is a DJA error as it doesn't do the permission check on the right object it returns)
2. For the relationship issue you might want to implement your custom ResourceRelatedField which overwrites get_queryset with your business/permission logic. Or otherwise address the issue in DRF directly how they would approach it.

[Anton-Shutik]

Fair point. I guess self.check_object_permissions(self.request, obj) should be run in get_related_instance before it returns instance. I guess this would solve your issue?

It is called in get_object. It will not help.
Initially, RelatedMixin was designed just to return related entities based on parent's permissions.
If you need some customer permissions, I think best way is have the related entity on its own viewset with its own permission.
In other words if you need permissions on /api/course/1/items/ different from permissions on /api/course/1/ you just need to replace in our code:

url('api/course/(?P<pk>\d+)/(?P<related_field>\w+)', CourseViewSet.as_view({'get': 'retrieve_related'}))

with

url('api/course/(?P<course_pk>\d+)/items', CourseItemViewSet.as_view({'get': 'list'}))

and check permissions in the CourseItemViewSet.

Also, I've created a small PR that probably handles some specific permissions for related entities. Probably somebody will find it helpful.

[n2ygk]

@Anton-Shutik Thanks. I'll take a look in the next few days and provide feedback.

[sliverc]

We should refrain from adding new api in DJA which are not DJA specific issues. And the issue we are discussing here I think is a DRF issue and not DJA.

This said:

1. Instead of Added related fields filtering #504 someone needs to do filtering in the serializer relation itself in their code base. See for instance here but instead of PrimaryKeyRelatedField it needs to derive from ResourceRelatedField in the DJA case. This also works with included_serializers.
2. For Added permission checks for related instances #505 in DRF permission_classes are defined per view and if different permission classes are needed then a new view is created. There is no specific permission checks for relations which are serialized in DRF either.
   Our RelatedMixin is no different the only difference is that relationships are served on a different url, but still using the same view. So my previous suggestion running check_object_permissions for related_instance also weakens this one permission per view approach and might be confusing.
   We still have in our documentation how to have one view for each relationship.
   So if someone wants to have different permissions on different relationships this is still possible (although as I understand idea in my 1st point would already address the initial request of this issue)

So I suggest we do not merge #504 #505 but might think about improving the documentation on this point.

[sliverc]

See also discussion at DRF encode/django-rest-framework#1985 covering the same issue.

[n2ygk]

@sliverc Agree w.r.t. not adding #504, #505 and improving documentation to include the requirement to not use RelatedMixin when not all child related fields have the same permissions as the parent.

@Anton-Shutik Sorry that this issue thread and #504 (comment) are covering the same conversation. I'll try to keep it here.

I guess my point in my #504 (comment) is that what I really need is a filter that checks permissions to decide if data should be retrieved. I don't want to return an error to the request because some of the related data is not permitted to be retrieved. Therefore it's a filter, not a permission, but it is based on "get" permissions. I haven't even thought about what trying to POST or PATCH those related fields would look like....

Would it make sense to add a RelatedMixin.retrieve_related_view or RelatedViewMixin.retrieve_related which uses the view rather than the serializer, thereby implementing the view's permissions, filters, etc? I am trying to get to a shorthand which RelatedMixin provides by not having to explicitly enumerate a URL pattern for every single relationship.

[Anton-Shutik]

Things getting complicated :) I think that retrieve_related breaks a concept one view for one resource type, since single view can return different resource types.
But this is a very important concept, that is followed by DRF. So probably we should continue overriding get_queryset and get_object in related views and check permissions there.

For your situation, I'm not sure what is the best way to handle it. Is it possible to filter out items that are not permitted in the filter and simply not return them ? At least client will get all items that they permitted to see

[Anton-Shutik]

@n2ygk
From DRF code:

class GenericAPIView(views.APIView):

    def get_object(self):
        ..........
        self.check_object_permissions(self.request, obj)

        return obj

probably we need change like this:

class ListModelMixin(object):
    """
    List a queryset.
    """
    def list(self, request, *args, **kwargs):
        queryset = self.filter_queryset(self.get_queryset())

        ########check each object############
        for i in queryset:
            self.check_object_permissions(i)
        ####################

       ...........
        return Response(serializer.data)

What do you think ?

[n2ygk]

Will have to take a look. FWIW here's what @lcary uses for ViewSets:

class ModelOrObjectPermissions(DjangoObjectPermissions):
    """
    Extends `DjangoObjectPermissions` with 'view' permissions.
    Additionally supports checking model-level and object-level
    permissions with the `check_object_permissions()` function.
    """
    perms_map = {
        'GET': ['%(app_label)s.view_%(model_name)s'],
        'OPTIONS': ['%(app_label)s.view_%(model_name)s'],
        'HEAD': ['%(app_label)s.view_%(model_name)s'],
        'POST': ['%(app_label)s.add_%(model_name)s'],
        'PUT': ['%(app_label)s.change_%(model_name)s'],
        'PATCH': ['%(app_label)s.change_%(model_name)s'],
        'DELETE': ['%(app_label)s.delete_%(model_name)s'],
    }

    def has_permission(self, request, view):
        """
        All views besides the `create()` view of the generic view types
        should check object-level permissions. Thus, this function defers
        all model-level checks, except for POST requests.

        Knobs:
         * For views that require the use of POST, but want to explicitly
           check only object-level permissions, the class variable
           `_ignore_model_permissions` must be set to `True`. This flag
           is used by the parent class.
         * For views that do not use the POST method, but want to explicitly
           check model permissions, `force_check_model_permissions` setting
           can be set to `True` on the view.
        """
        setting = 'force_check_model_permissions'
        force_check_model_permissions = getattr(view, setting, False)
        if request.method == 'POST' or force_check_model_permissions is True:
            return super().has_permission(request, view)
        else:
            # defer the check to `has_object_permission()`
            return True

    def has_object_permission(self, request, view, obj):
        """
        Allows for checking if the user has permissions at the model-level
        or object level.

        This differs from the `DjangoObjectPermissions` class, which
        typically only checks permissions at the object-level when
        `check_object_permissions()` is called. This class also checks model-
        level permissions.

        The user is considered as having permissions to take an action on an
        object if they either have permissions for all instances (aka model-
        level) of an object, or they have permission to a specific instance
        (aka object-level).
        """
        setting = '_ignore_model_permissions'
        ignore_check_model_permissions = getattr(view, setting, False)
        if (not ignore_check_model_permissions and
                super().has_permission(request, view)):
            # This means the user/group has model permissions:
            return True
        # Otherwise, verify object permissions:
        return super().has_object_permission(request, view, obj)


class ModelOrObjectPermissionsFilter(BaseFilterBackend):
    """
    A filter backend that limits results to those where the requesting user
    has read object level permissions.

    Extends django-rest-framework's DjangoObjectPermissionsFilter,
    except does not enforce the accept_global_perms=False rule.
    """
    perm_format = '%(app_label)s.view_%(model_name)s'

    def filter_queryset(self, request, queryset, view):
        user = request.user
        model_cls = queryset.model
        kwargs = {
            'app_label': model_cls._meta.app_label,
            'model_name': model_cls._meta.model_name
        }
        permission = self.perm_format % kwargs
        return get_objects_for_user(user, permission, queryset)

[sliverc]

@n2ygk I am passing through all the open issues and this discussion seems to have stalled and it is not quite clear what the result of this issue actually should be at this point as it seems rather a DRF issue. Do you agree that we close this issue?

[sliverc]

As there hasn't been any feedback I am closing this issue. Please comment though if you think this issue should remain open.