fix: prevent overwrite of hidden relations in patch

README.md

@@ -19,26 +19,23 @@ specific rules as to who can write posts.
 DGAP makes it easy to implement the blog once, and make the permissions,
 visibilities, and validations custom to each deployment.
 
-
 ## Concepts
 
 DGAP provides you with three configuration settings: Visibilities, Permissions,
 and Validations.
 
-* The **visibilities** are run when getting data from the API. They define, on a
+- The **visibilities** are run when getting data from the API. They define, on a
   per-user base, who can see which data.
-* The **validations** are run on create and update operations, so
+- The **validations** are run on create and update operations, so
   data can be checked and modified before the update takes place.
-* The **permissions** then define what a user can do with a given (visible) piece of
+- The **permissions** then define what a user can do with a given (visible) piece of
   data.
 
-
 ## Installation for app developers
 
 If you want to integrate DGAP into your app, these are the steps you need.
 Install DGAP (and add to your requirements files etc) first.
 
-
 ```bash
 pip install django-generic-api-permissions
 ```
@@ -75,16 +72,26 @@ class MyModelViewset(VisibilityViewMixin, ModelViewSet):
 
 Data leaks in REST Framework may happen if you use includes (or even only references) from a model the user may see to something that should be hidden.
 To avoid such leaks, make sure to use a subclassed related field (either by creating your own using the provided `VisibilityRelatedFieldMixin`, or by using one of the provided types. See example below).
+To set it for every relation in the serializer use DRFs `serializer_related_field` attribute in the serializer.
+
+It's important to be aware of potential issues when updating (PATCH) existing relationships, especially when some relationships are hidden due to visibility settings. If not handled correctly, the hidden relationships may be unintentionally removed during an update, resulting in only the new relationships being set.
+
+To avoid this, you must incorporate the `VisibilitySerializerMixin` into your serializer where you're using the `VisibilityRelatedFieldMixin` for the relationship field. This ensures that hidden relationships are properly accounted for during updates.
+
+Remember to define the `VisibilitySerializerMixin` after the `ValidatorMixin`. This order is crucial because it ensures that validations are performed first, and only then are the relationships updated.
+
+This step is vital to maintain the integrity of your data and prevent accidental loss of hidden relationships.
 
 ```python
 # serializers.py
 from rest_framework.viewsets import ModelSerializer
-from generic_permissions.visibilities import VisibilityPrimaryKeyRelatedField
-class MyModelSerializers(ModelSerializer):
+from generic_permissions.visibilities import VisibilityPrimaryKeyRelatedField, VisibilitySerializerMixin
+class MyModelSerializers(VisibilitySerializerMixin, ModelSerializer):
     serializer_related_field = VisibilityPrimaryKeyRelatedField
 ```
 
-A few subclassed fields are provided:
+A few subclassed fields are provided for different types of `RelatedField`:
+
 - `VisibilityPrimaryKeyRelatedField`
 - `VisibilityResourceRelatedField`
 - `VisibilitySerializerMethodResourceRelatedField`
@@ -110,12 +117,13 @@ class MyModelViewset(PermissionViewMixin, VisibilityViewMixin, ModelViewSet):
     serializer_class = ...
     queryset = ...
 ```
-You may use only one of the two mixins, or both, depending on your needs.
 
+You may use only one of the two mixins, or both, depending on your needs.
 
 ### Validation subsystem
 
 Last, for the validation system, you extend your **serializer** with a mixin:
+
 ```python
 # serializers.py
 from rest_framework.serializers import ModelSerializer
@@ -124,14 +132,14 @@ from generic_permissions.serializers import PermissionSerializerMixin
 from generic_permissions.validation import ValidatorMixin
 
 from myapp import models
+
 class MyModelSerializer(ValidatorMixin, ModelSerializer):
     # my field definitions...
     class Meta:
         model = models.MyModel
         fields = "__all__"
 ```
 
-
 ## Usage - for people deploying a DGAP-equipped app
 
 Say you have an blog you want to deploy that uses DGAP. You want public blog
@@ -142,6 +150,7 @@ accordingly.
 ### Visibilities
 
 First, let's define the visibility class:
+
 ```python
 # my_custom_visibilities.py
 from generic_permissions.visibilities import filter_queryset_for
@@ -192,7 +201,6 @@ class ResultingVisibility(Union):
     visibility_classes = [MyFirstVisibility, MySecondVisibility]
 ```
 
-
 ### Permissions
 
 Permission classes define who may perform which data mutation. They can be configured
@@ -202,10 +210,11 @@ To write custom permission classes, you create a simple class, and decorate the
 methods that define the permissions accordingly.
 
 There are two types of methods in the permissions system:
-* `permission_for`: Marks methods that define generic access permissions for a
+
+- `permission_for`: Marks methods that define generic access permissions for a
   given model. They are always checked first.
   Those methods will receive one positional argument, namely the `request` object
-* `object_permission_for`: Define whether access to a specific object shall be
+- `object_permission_for`: Define whether access to a specific object shall be
   granted. This called for all other operations **except** creation.
   These methods will receive two positional arguments: First, the `request`
   object, and second, the model instance that is being accessed in the request.
@@ -221,7 +230,7 @@ that to avoid code duplication.
 You can find more information about the `request` object in the
 [Django documentation](https://docs.djangoproject.com/en/3.1/ref/request-response/#httprequest-objects)
 
-``` python
+```python
 from generic_permissions.permissions import permission_for, object_permission_for
 from my_app.models import Post, Comment
 
@@ -230,7 +239,7 @@ class OnlyAuthenticated:
     def has_permission_default(self, request):
         # No permission is granted for any non-authenticated users
         return request.user.is_authenticated
-        
+
 class BlogPermissions:
     @permission_for(Comment)
     def has_permission_for_comment(self, request):
@@ -247,12 +256,12 @@ class BlogPermissions:
 ```
 
 The following pre-defined classes are available:
-* `generic_permissions.permissions.AllowAny`: allow any users to perform any mutation (default)
-* `generic_permissions.permissions.DenyAll`: deny all operations to any object.
+
+- `generic_permissions.permissions.AllowAny`: allow any users to perform any mutation (default)
+- `generic_permissions.permissions.DenyAll`: deny all operations to any object.
   You can use this as a base class for your permissions - as long as you don't
   allow something, it will be denied.
 
-
 ### Data validation
 
 Once the permission to access or modify an object is granted, you may want to
@@ -285,4 +294,4 @@ the method returns a `dict` with a compatible structure. You may also
 to succeed.
 The second parameter, `context`, is a `dict` containing the DRF context: Access
 `context['request']` to get the request (if validation depends on the user,
-for example). 
+for example).

generic_permissions/visibilities.py

@@ -60,6 +60,46 @@ def get_attribute(self, instance):
         return queryset
 
 
+class VisibilitySerializerMixin:
+    """
+    Mixin for serializers to handle visibility of related fields.
+
+    This mixin ensures that when updating (PATCH) existing relationships,
+    any relationships not included in the request (due to visibility settings)
+    are not unintentionally removed. It does this by adding back the existing relations
+    which are not included in the request.
+
+    This mixin should be used in conjunction with the `VisibilityRelatedFieldMixin`
+    for the relationship field and should be defined after the `ValidatorMixin`
+    to ensure validations are performed first.
+    """
+
+    def validate(self, *args, **kwargs):
+        validated_data = super().validate(*args, **kwargs)
+
+        if not self.instance:
+            return validated_data
+
+        # Incoming patches can have a subset of all relations, so we need to
+        # keep the existing relations which are not included in the request.
+        for key, field in self.fields.items():
+            if (
+                type(field) is not VisibilityManyRelatedField
+                or key not in validated_data
+            ):
+                continue
+
+            # Find the relations which the request can include.
+            queryset = getattr(self.instance, key).all()
+            for handler in VisibilitiesConfig.get_handlers(queryset.model):
+                queryset = handler(queryset, self.context)
+
+            # Add remaining relations which can not be included in the request.
+            validated_data[key] += getattr(self.instance, key).exclude(pk__in=queryset)
+
+        return validated_data
+
+
 class VisibilityRelatedFieldMixin:
     @classmethod
     def many_init(cls, *args, **kwargs):
@@ -88,6 +128,17 @@ def get_attribute(self, instance):
 
         return model_instance
 
+    def bind(self, field_name, parent):
+        parent_cls = type(parent)
+        if parent_cls is VisibilityManyRelatedField:
+            return super().bind(field_name, parent)
+
+        if VisibilitySerializerMixin not in parent_cls.mro():
+            raise RuntimeWarning(
+                f"To avoid data loss, use VisibilitySerializerMixin in {parent_cls.__name__}"
+            )
+        return super().bind(field_name, parent)
+
 
 class VisibilityPrimaryKeyRelatedField(
     VisibilityRelatedFieldMixin, PrimaryKeyRelatedField

tests/serializers.py

@@ -1,12 +1,17 @@
 from rest_framework import serializers
 
 from generic_permissions.validation import ValidatorMixin
-from generic_permissions.visibilities import VisibilityPrimaryKeyRelatedField
+from generic_permissions.visibilities import (
+    VisibilityPrimaryKeyRelatedField,
+    VisibilitySerializerMixin,
+)
 
 from . import models
 
 
-class TestModel1Serializer(ValidatorMixin, serializers.ModelSerializer):
+class TestModel1Serializer(
+    ValidatorMixin, VisibilitySerializerMixin, serializers.ModelSerializer
+):
     serializer_related_field = VisibilityPrimaryKeyRelatedField
     explicit = VisibilityPrimaryKeyRelatedField(
         queryset=models.Model1.objects.all(), many=True, source="many"

tests/test_visibilities.py

@@ -7,6 +7,13 @@
 from generic_permissions.visibilities import Union, filter_queryset_for
 
 from .models import BaseModel, Model1, Model2
+from rest_framework import serializers
+
+from generic_permissions.validation import ValidatorMixin
+from generic_permissions.visibilities import (
+    VisibilityPrimaryKeyRelatedField,
+    VisibilitySerializerMixin,
+)
 
 
 @pytest.mark.parametrize("detail", [True, False])
@@ -246,3 +253,52 @@ def filter_queryset_for_document(self, queryset, request):
         assert len(result["explicit"]) == 1
         assert model2.pk in result["many"]
         assert model2.pk in result["explicit"]
+
+
+def test_visibility_relation_patch(db, admin_user, admin_client):
+    class TestVisibility:
+        @filter_queryset_for(Model2)
+        def filter_queryset_for_document(self, queryset, request):
+            return queryset.exclude(text="apple")
+
+    VisibilitiesConfig.clear_handlers()
+    VisibilitiesConfig.register_handler_class(TestVisibility)
+
+    model1 = Model1.objects.create(text="pear")
+    Model2.objects.create(text="hidden")
+    model2 = Model2.objects.create(text="apple")
+    model3 = Model2.objects.create(text="melon")
+    model4 = Model2.objects.create(text="orange")
+    model1.many.add(model2)
+    model1.many.add(model3)
+
+    data = {"many": [model4.pk]}
+
+    url = reverse("model1-detail", args=[model1.pk])
+    response = admin_client.patch(url, data)
+
+    assert response.status_code == HTTP_200_OK
+    result = response.json()
+
+    assert result["text"] == "pear"
+    assert len(result["many"]) == 1
+    assert model4.pk in result["many"]
+
+    model1.refresh_from_db()
+    assert model1.many.all().count() == 2
+    assert model1.many.filter(pk=model2.pk).exists()
+    assert model1.many.filter(pk=model4.pk).exists()
+
+
+def test_visibility_related_field_check(db):
+    class WrongSerializer(serializers.ModelSerializer):
+        serializer_related_field = VisibilityPrimaryKeyRelatedField
+
+        class Meta:
+            model = Model1
+            fields = "__all__"
+
+    model1 = Model1.objects.create(text="pear")
+    with pytest.raises(RuntimeWarning):
+        serializer = WrongSerializer(model1)
+        serializer.data