From dd0d36a7677c11123145b7075d7b3c0aff475c9a Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 4 Jan 2024 16:51:46 +1030 Subject: [PATCH] feat: adds user_permissions to the tagging REST API results These permissions reflect the current request.user's allowed actions on the instances in the results list. --- .../core/tagging/rest_api/v1/serializers.py | 88 ++++++++++++- .../core/tagging/rest_api/v1/views.py | 2 +- .../core/tagging/test_views.py | 119 +++++++++++++++--- 3 files changed, 188 insertions(+), 21 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index 371e5ce7..a9a4c710 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -30,11 +30,71 @@ class TaxonomyExportQueryParamsSerializer(serializers.Serializer): # pylint: di output_format = serializers.RegexField(r"(?i)^(json|csv)$", allow_blank=False) +class UserPermissionsSerializer(serializers.Serializer): # pylint: disable=abstract-method + """ + Serializer for the permissions on tagging instances granted to the current user. + + Requires the current request to be passed into the serializer context, or all permissions will return False. + """ + can_add = serializers.SerializerMethodField() + can_view = serializers.SerializerMethodField() + can_change = serializers.SerializerMethodField() + can_delete = serializers.SerializerMethodField() + + def _check_permission(self, action, instance) -> bool: + """ + Returns True if the current `request.user` may perform the given `action` on the `instance` object. + + Uses the current request as passed into the serializer context. + """ + assert action in ("add", "change", "view", "delete") + request = self.context.get('request') + if not (request and request.user and instance): + return False + + if isinstance(instance, Taxonomy): + model = 'taxonomy' + elif isinstance(instance, Tag): + model = 'tag' + elif isinstance(instance, ObjectTag): + model = 'objecttag' + else: + return False + + permission = f"oel_tagging.{action}_{model}" + return request.user.has_perm(permission, instance) + + def get_can_add(self, instance) -> bool: + """ + Returns True if the current user is allowed to create new instances. + """ + return self._check_permission('add', instance) + + def get_can_change(self, instance) -> bool: + """ + Returns True if the current user is allowed to change this instance. + """ + return self._check_permission('change', instance) + + def get_can_view(self, instance) -> bool: + """ + Returns True if the current user is allowed to view this instance. + """ + return self._check_permission('view', instance) + + def get_can_delete(self, instance) -> bool: + """ + Returns True if the current user is allowed to delete this instance. + """ + return self._check_permission('delete', instance) + + class TaxonomySerializer(serializers.ModelSerializer): """ Serializer for the Taxonomy model. """ tags_count = serializers.SerializerMethodField() + user_permissions = serializers.SerializerMethodField() class Meta: model = Taxonomy @@ -48,6 +108,7 @@ class Meta: "system_defined", "visible_to_authors", "tags_count", + "user_permissions", ] def to_representation(self, instance): @@ -60,6 +121,13 @@ def to_representation(self, instance): def get_tags_count(self, instance): return instance.tag_set.count() + def get_user_permissions(self, instance): + """ + Returns the serialized user permissions on the given instance. + """ + permissions = UserPermissionsSerializer(instance, context=self.context) + return permissions.to_representation(instance) + class ObjectTagListQueryParamsSerializer(serializers.Serializer): # pylint: disable=abstract-method """ @@ -78,9 +146,17 @@ class ObjectTagMinimalSerializer(serializers.ModelSerializer): class Meta: model = ObjectTag - fields = ["value", "lineage"] + fields = ["value", "lineage", "user_permissions"] lineage = serializers.ListField(child=serializers.CharField(), source="get_lineage", read_only=True) + user_permissions = serializers.SerializerMethodField() + + def get_user_permissions(self, instance): + """ + Returns the serialized user permissions on the given instance. + """ + permissions = UserPermissionsSerializer(instance, context=self.context) + return permissions.to_representation(instance) class ObjectTagSerializer(ObjectTagMinimalSerializer): @@ -122,7 +198,7 @@ def to_representation(self, instance: list[ObjectTag]) -> dict: "tags": [] } taxonomies.append(tax_entry) - tax_entry["tags"].append(ObjectTagMinimalSerializer(obj_tag).data) + tax_entry["tags"].append(ObjectTagMinimalSerializer(obj_tag, context=self.context).data) return by_object @@ -161,6 +237,7 @@ class TagDataSerializer(serializers.Serializer): # pylint: disable=abstract-met _id = serializers.IntegerField(allow_null=True) sub_tags_url = serializers.SerializerMethodField() + user_permissions = serializers.SerializerMethodField() def get_sub_tags_url(self, obj: TagData | Tag): """ @@ -184,6 +261,13 @@ def get_sub_tags_url(self, obj: TagData | Tag): return request.build_absolute_uri(url) return None + def get_user_permissions(self, instance): + """ + Returns the serialized user permissions on the given instance. + """ + permissions = UserPermissionsSerializer(instance, context=self.context) + return permissions.to_representation(instance) + def to_representation(self, instance: TagData | Tag) -> dict: """ Convert this TagData (or Tag model instance) to the serialized dictionary diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 6d685d4e..06a0d32b 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -444,7 +444,7 @@ def retrieve(self, request, *args, **kwargs) -> Response: behavior we want. """ object_tags = self.filter_queryset(self.get_queryset()) - serializer = ObjectTagsByTaxonomySerializer(list(object_tags)) + serializer = ObjectTagsByTaxonomySerializer(list(object_tags), context=self.get_serializer_context()) response_data = serializer.data if self.kwargs["object_id"] not in response_data: # For consistency, the key with the object_id should always be present in the response, even if there diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 82e68588..a79c7746 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -53,6 +53,7 @@ def check_taxonomy( allow_free_text=False, system_defined=False, visible_to_authors=True, + user_permissions=None, ): """ Check taxonomy data @@ -65,6 +66,7 @@ def check_taxonomy( assert data["allow_free_text"] == allow_free_text assert data["system_defined"] == system_defined assert data["visible_to_authors"] == visible_to_authors + assert data["user_permissions"] == user_permissions class TestTaxonomyViewMixin(APITestCase): @@ -129,10 +131,10 @@ def test_list_taxonomy_queryparams(self, enabled, expected_status: int, expected @ddt.data( (None, status.HTTP_401_UNAUTHORIZED, 0), ("user", status.HTTP_200_OK, 10), - ("staff", status.HTTP_200_OK, 20), + ("staff", status.HTTP_200_OK, 20, True), ) @ddt.unpack - def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_count: int): + def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_count: int, is_admin=False): taxonomy = api.create_taxonomy(name="Taxonomy enabled 1", enabled=True) for i in range(tags_count): tag = Tag( @@ -163,6 +165,13 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c "system_defined": True, "visible_to_authors": True, "tags_count": 0, + # System taxonomy cannot be modified + "user_permissions": { + "can_add": False, + "can_view": True, + "can_change": False, + "can_delete": False, + }, }, { "id": taxonomy.id, @@ -174,6 +183,13 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c "system_defined": False, "visible_to_authors": True, "tags_count": tags_count, + # Enabled taxonomy can be modified by staff + "user_permissions": { + "can_add": is_admin, + "can_view": True, + "can_change": is_admin, + "can_delete": is_admin, + }, }, ] @@ -225,6 +241,12 @@ def test_language_taxonomy(self): description="Languages that are enabled on this system.", allow_multiple=False, # We may change this in the future to allow multiple language tags system_defined=True, + user_permissions={ + "can_add": False, + "can_view": True, + "can_change": False, + "can_delete": False, + }, ) @ddt.data( @@ -232,11 +254,13 @@ def test_language_taxonomy(self): (None, {"enabled": False}, status.HTTP_401_UNAUTHORIZED), ("user", {"enabled": True}, status.HTTP_200_OK), ("user", {"enabled": False}, status.HTTP_404_NOT_FOUND), - ("staff", {"enabled": True}, status.HTTP_200_OK), - ("staff", {"enabled": False}, status.HTTP_200_OK), + ("staff", {"enabled": True}, status.HTTP_200_OK, True), + ("staff", {"enabled": False}, status.HTTP_200_OK, True), ) @ddt.unpack - def test_detail_taxonomy(self, user_attr: str | None, taxonomy_data: dict[str, bool], expected_status: int): + def test_detail_taxonomy( + self, user_attr: str | None, taxonomy_data: dict[str, bool], expected_status: int, is_admin=False, + ): create_data = {"name": "taxonomy detail test", **taxonomy_data} taxonomy = api.create_taxonomy(**create_data) # type: ignore[arg-type] url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) @@ -249,7 +273,14 @@ def test_detail_taxonomy(self, user_attr: str | None, taxonomy_data: dict[str, b assert response.status_code == expected_status if status.is_success(expected_status): - check_taxonomy(response.data, taxonomy.pk, **create_data) + expected_data = create_data + expected_data["user_permissions"] = { + "can_add": is_admin, + "can_view": True, + "can_change": is_admin, + "can_delete": is_admin, + } + check_taxonomy(response.data, taxonomy.pk, **expected_data) def test_detail_system_taxonomy(self): url = TAXONOMY_DETAIL_URL.format(pk=LANGUAGE_TAXONOMY_ID) @@ -297,11 +328,18 @@ def test_create_taxonomy(self, user_attr: str | None, expected_status: int): # If we were able to create the taxonomy, check if it was created if status.is_success(expected_status): - check_taxonomy(response.data, response.data["id"], **create_data) + expected_data = create_data + expected_data["user_permissions"] = { + "can_add": True, + "can_view": True, + "can_change": True, + "can_delete": True, + } + check_taxonomy(response.data, response.data["id"], **expected_data) url = TAXONOMY_DETAIL_URL.format(pk=response.data["id"]) response = self.client.get(url) - check_taxonomy(response.data, response.data["id"], **create_data) + check_taxonomy(response.data, response.data["id"], **expected_data) @ddt.data( {}, @@ -358,6 +396,12 @@ def test_update_taxonomy(self, user_attr, expected_status): "name": "new name", "description": "taxonomy description", "enabled": True, + "user_permissions": { + "can_add": True, + "can_view": True, + "can_change": True, + "can_delete": True, + }, }, ) @@ -414,6 +458,12 @@ def test_patch_taxonomy(self, user_attr, expected_status): **{ "name": "new name", "enabled": True, + "user_permissions": { + "can_add": True, + "can_view": True, + "can_change": True, + "can_delete": True, + }, }, ) @@ -580,7 +630,7 @@ def _change_object_permission(user, object_id: str) -> bool: """ For testing, let everyone have edit object permission on object_id "abc" and "limit_tag_count" """ - if object_id in ("abc", "limit_tag_count"): + if object_id in ("abc", "limit_tag_count", "problem7", "problem15", "html7"): return True return can_change_object_tag_objectid(user, object_id) @@ -647,10 +697,22 @@ def test_retrieve_object_tags(self, user_attr, expected_status): { "value": "Mammalia", "lineage": ["Eukaryota", "Animalia", "Chordata", "Mammalia"], + "user_permissions": { + "can_add": True, + "can_view": True, + "can_change": True, + "can_delete": True, + }, }, { "value": "Fungi", "lineage": ["Eukaryota", "Fungi"], + "user_permissions": { + "can_add": True, + "can_view": True, + "can_change": True, + "can_delete": True, + }, }, ] }, @@ -662,6 +724,12 @@ def test_retrieve_object_tags(self, user_attr, expected_status): { "value": "test_user_1", "lineage": ["test_user_1"], + "user_permissions": { + "can_add": True, + "can_view": True, + "can_change": True, + "can_delete": True, + }, }, ], } @@ -687,6 +755,15 @@ def prepare_for_sort_test(self) -> tuple[str, list[dict]]: "1111-grandchild", ] + shared_props = { + "user_permissions": { + "can_add": True, + "can_view": True, + "can_change": True, + "can_delete": True, + }, + } + # Apply the object tags: taxonomy = self.create_sort_test_taxonomy() api.tag_object(object_id=object_id, taxonomy=taxonomy, tags=sort_test_tags) @@ -713,15 +790,15 @@ def prepare_for_sort_test(self) -> tuple[str, list[dict]]: # ANVIL # azure sort_test_applied_result = [ - {"value": "1 A", "lineage": ["1", "1 A"]}, - {"value": "1111-grandchild", "lineage": ["1", "11111", "1111-grandchild"]}, - {"value": "111", "lineage": ["111"]}, - {"value": "11111111", "lineage": ["111", "11111111"]}, - {"value": "123", "lineage": ["111", "123"]}, - {"value": "abstract", "lineage": ["abstract"]}, - {"value": "azores islands", "lineage": ["abstract", "azores islands"]}, - {"value": "Android", "lineage": ["ALPHABET", "Android"]}, - {"value": "ANVIL", "lineage": ["ALPHABET", "ANVIL"]}, + {"value": "1 A", "lineage": ["1", "1 A"], **shared_props}, + {"value": "1111-grandchild", "lineage": ["1", "11111", "1111-grandchild"], **shared_props}, + {"value": "111", "lineage": ["111"], **shared_props}, + {"value": "11111111", "lineage": ["111", "11111111"], **shared_props}, + {"value": "123", "lineage": ["111", "123"], **shared_props}, + {"value": "abstract", "lineage": ["abstract"], **shared_props}, + {"value": "azores islands", "lineage": ["abstract", "azores islands"], **shared_props}, + {"value": "Android", "lineage": ["ALPHABET", "Android"], **shared_props}, + {"value": "ANVIL", "lineage": ["ALPHABET", "ANVIL"], **shared_props}, ] return object_id, sort_test_applied_result @@ -802,6 +879,12 @@ def test_retrieve_object_tags_taxonomy_queryparam( { "value": "test_user_1", "lineage": ["test_user_1"], + "user_permissions": { + "can_add": True, + "can_view": True, + "can_change": True, + "can_delete": True, + }, }, ], }