From f36b0d684648f569a7cf69dd6de9a7814f86d3b1 Mon Sep 17 00:00:00 2001 From: yelinz Date: Wed, 20 Dec 2023 08:58:16 +0100 Subject: [PATCH] fix: prevent overwrite of hidden relations in patch --- README.md | 45 ++++++++++++++++------------- generic_permissions/visibilities.py | 27 +++++++++++++++++ tests/serializers.py | 13 +++++++-- tests/test_visibilities.py | 35 ++++++++++++++++++++++ 4 files changed, 97 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index bf59acd..c7b1a47 100644 --- a/README.md +++ b/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 ``` @@ -76,15 +73,21 @@ 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). +Similar problems can occur when PATCHing existing relationships where some are hidden through the visibilities. +The hidden relations will be overwritten by the latest update. +To prevent that include the `VisibilitySerializerMixin` in your serializer where you use `ManyToMany` relations. +Preferably after the `ValidatorMixin` so that validations first run and only afterwards it considers PATCHing the relation. + ```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 +113,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,6 +128,7 @@ 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: @@ -131,7 +136,6 @@ class MyModelSerializer(ValidatorMixin, ModelSerializer): 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 +146,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 +197,6 @@ class ResultingVisibility(Union): visibility_classes = [MyFirstVisibility, MySecondVisibility] ``` - ### Permissions Permission classes define who may perform which data mutation. They can be configured @@ -202,10 +206,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 +226,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 +235,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 +252,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 +290,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). diff --git a/generic_permissions/visibilities.py b/generic_permissions/visibilities.py index 2011674..e4ffffa 100644 --- a/generic_permissions/visibilities.py +++ b/generic_permissions/visibilities.py @@ -60,6 +60,33 @@ def get_attribute(self, instance): return queryset +class VisibilitySerializerMixin: + 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): diff --git a/tests/serializers.py b/tests/serializers.py index 0ff3b50..18734b7 100644 --- a/tests/serializers.py +++ b/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" @@ -17,7 +22,9 @@ class Meta: fields = "__all__" -class TestModel2Serializer(ValidatorMixin, serializers.ModelSerializer): +class TestModel2Serializer( + ValidatorMixin, VisibilitySerializerMixin, serializers.ModelSerializer +): class Meta: model = models.Model2 fields = "__all__" diff --git a/tests/test_visibilities.py b/tests/test_visibilities.py index c2e7f08..3e365b1 100644 --- a/tests/test_visibilities.py +++ b/tests/test_visibilities.py @@ -246,3 +246,38 @@ 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()