From 05ea800ecb43ef93e46b691d5595e7bd84c7e716 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 4 Jan 2024 16:17:56 +1030 Subject: [PATCH 01/17] chore: removed unneeded code to get test coverage back to 100% for this file. --- .../core/tagging/rest_api/v1/serializers.py | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index a875c98b..371e5ce7 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -144,7 +144,7 @@ class ObjectTagUpdateQueryParamsSerializer(serializers.Serializer): # pylint: d ) -class TagDataSerializer(serializers.Serializer): +class TagDataSerializer(serializers.Serializer): # pylint: disable=abstract-method """ Serializer for TagData dicts. Also can serialize Tag instances. @@ -194,12 +194,6 @@ def to_representation(self, instance: TagData | Tag) -> dict: data["parent_value"] = instance.parent.value if instance.parent else None return data - def update(self, instance, validated_data): - raise RuntimeError('`update()` is not supported by the TagData serializer.') - - def create(self, validated_data): - raise RuntimeError('`create()` is not supported by the TagData serializer.') - class TaxonomyTagCreateBodySerializer(serializers.Serializer): # pylint: disable=abstract-method """ @@ -273,7 +267,7 @@ class Meta: ] -class TaxonomyImportPlanResponseSerializer(serializers.Serializer): +class TaxonomyImportPlanResponseSerializer(serializers.Serializer): # pylint: disable=abstract-method """ Serializer for the response of the Taxonomy Import Plan request """ @@ -290,9 +284,3 @@ def get_plan(self, obj): return plan.plan() return None - - def update(self, instance, validated_data): - raise RuntimeError('`update()` is not supported by the TagImportTask serializer.') - - def create(self, validated_data): - raise RuntimeError('`create()` is not supported by the TagImportTask serializer.') From 231036fec4cd41c0e3a410ab988a00fd526f1e24 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 4 Jan 2024 16:51:46 +1030 Subject: [PATCH 02/17] 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..d35df865 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") + + if isinstance(instance, Taxonomy): + model = 'taxonomy' + elif isinstance(instance, Tag): + model = 'tag' + elif isinstance(instance, ObjectTag): + model = 'objecttag' + else: + return False + + request = self.context.get('request') + assert request and request.user + + 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, + }, }, ], } From 9df53042a10244ef81ff8873fa20f65da69c234a Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Mon, 8 Jan 2024 01:45:28 +1030 Subject: [PATCH 03/17] fix: removes ObjectTagsByTaxonomySerializer.editable frontend will use userPermissions instead --- openedx_tagging/core/tagging/rest_api/v1/serializers.py | 1 - tests/openedx_tagging/core/tagging/test_views.py | 3 --- 2 files changed, 4 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index d35df865..e4aa0d0b 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -194,7 +194,6 @@ def to_representation(self, instance: list[ObjectTag]) -> dict: tax_entry = { "name": obj_tag.name, "taxonomy_id": obj_tag.taxonomy_id, - "editable": (not obj_tag.taxonomy.cast().system_defined) if obj_tag.taxonomy else False, "tags": [] } taxonomies.append(tax_entry) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index a79c7746..a19be95b 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -690,7 +690,6 @@ def test_retrieve_object_tags(self, user_attr, expected_status): { "name": "Life on Earth", "taxonomy_id": 1, - "editable": True, "tags": [ # Note: based on tree order (Animalia before Fungi), this tag comes first even though it # starts with "M" and Fungi starts with "F" @@ -719,7 +718,6 @@ def test_retrieve_object_tags(self, user_attr, expected_status): { "name": "User Authors", "taxonomy_id": 3, - "editable": False, "tags": [ { "value": "test_user_1", @@ -874,7 +872,6 @@ def test_retrieve_object_tags_taxonomy_queryparam( { "name": "User Authors", "taxonomy_id": 3, - "editable": False, "tags": [ { "value": "test_user_1", From 8d619c5b04d561ae0c4d53ce16e0503e997037dd Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Mon, 8 Jan 2024 05:54:28 +1030 Subject: [PATCH 04/17] fix: determine "add" permissions without the object --- openedx_tagging/core/tagging/rest_api/v1/serializers.py | 2 ++ tests/openedx_tagging/core/tagging/test_views.py | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index e4aa0d0b..a393e956 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -61,6 +61,8 @@ def _check_permission(self, action, instance) -> bool: request = self.context.get('request') assert request and request.user + if action == 'add': + instance = None permission = f"oel_tagging.{action}_{model}" return request.user.has_perm(permission, instance) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index a19be95b..c7766a3f 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -167,7 +167,7 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c "tags_count": 0, # System taxonomy cannot be modified "user_permissions": { - "can_add": False, + "can_add": is_admin, "can_view": True, "can_change": False, "can_delete": False, From 23789cc68ff76fae47ea1af669be4895ad25a2ba Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Mon, 8 Jan 2024 10:02:02 +1030 Subject: [PATCH 05/17] feat: adds model-level user permissions to the Taxonomy and Tag REST APIs Uses a custom paginator to add this extra data into the response. --- .../core/tagging/rest_api/paginators.py | 68 ++++++- .../core/tagging/rest_api/v1/views.py | 3 +- .../core/tagging/test_views.py | 174 +++++++++++++++++- 3 files changed, 237 insertions(+), 8 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/paginators.py b/openedx_tagging/core/tagging/rest_api/paginators.py index 2ebce12d..abcd5ba6 100644 --- a/openedx_tagging/core/tagging/rest_api/paginators.py +++ b/openedx_tagging/core/tagging/rest_api/paginators.py @@ -1,14 +1,66 @@ """ Paginators uses by the REST API """ +from typing import Type from edx_rest_framework_extensions.paginators import DefaultPagination # type: ignore[import] +from rest_framework.response import Response + +from openedx_tagging.core.tagging.models import Tag, Taxonomy # From this point, the tags begin to be paginated MAX_FULL_DEPTH_THRESHOLD = 10_000 -class TagsPagination(DefaultPagination): +class ModelPermissionsPaginationMixin: + """ + Inserts model permissions data into the top level of the paginated response. + """ + permission_field_name = 'user_permissions' + permission_actions = ('add', 'view', 'change', 'delete') + + def get_model(self) -> Type: + """ + Returns the model that is being paginated. + """ + raise NotImplementedError # pragma: no cover + + def get_model_permissions(self) -> dict: + """ + Returns a dict containing the request user's permissions for the current model. + + The dict contains keys named `can_`, mapped to a boolean flag. + """ + user = self.request.user # type: ignore[attr-defined] + model = self.get_model() + app_label = model._meta.app_label + model_name = model._meta.model_name + return { + f'can_{action}': user.has_perm(f'{app_label}.{action}_{model_name}') + for action in self.permission_actions + } + + def get_paginated_response(self, data) -> Response: + """ + Injects the user's model-level permissions into the paginated response. + """ + response_data = super().get_paginated_response(data).data # type: ignore[misc] + response_data[self.permission_field_name] = self.get_model_permissions() + return Response(response_data) + + +class TaxonomyPagination(ModelPermissionsPaginationMixin, DefaultPagination): + """ + Inserts permissions data for Taxonomies into the top level of the paginated response. + """ + def get_model(self) -> Type: + """ + Returns the model that is being paginated. + """ + return Taxonomy + + +class TagsPagination(ModelPermissionsPaginationMixin, DefaultPagination): """ Custom pagination configuration for taxonomies with a large number of tags. Used on the get tags API view. @@ -16,8 +68,14 @@ class TagsPagination(DefaultPagination): page_size = 10 max_page_size = 300 + def get_model(self) -> Type: + """ + Returns the model that is being paginated. + """ + return Tag + -class DisabledTagsPagination(DefaultPagination): +class DisabledTagsPagination(ModelPermissionsPaginationMixin, DefaultPagination): """ Custom pagination configuration for taxonomies with a small number of tags. Used on the get tags API view @@ -28,3 +86,9 @@ class DisabledTagsPagination(DefaultPagination): """ page_size = MAX_FULL_DEPTH_THRESHOLD max_page_size = MAX_FULL_DEPTH_THRESHOLD + 1 + + def get_model(self) -> Type: + """ + Returns the model that is being paginated. + """ + return Tag diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 06a0d32b..42192bc6 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -30,7 +30,7 @@ from ...import_export.parsers import ParserFormat from ...models import Taxonomy from ...rules import ObjectTagPermissionItem -from ..paginators import MAX_FULL_DEPTH_THRESHOLD, DisabledTagsPagination, TagsPagination +from ..paginators import MAX_FULL_DEPTH_THRESHOLD, DisabledTagsPagination, TagsPagination, TaxonomyPagination from .permissions import ObjectTagObjectPermissions, TagObjectPermissions, TaxonomyObjectPermissions from .serializers import ( ObjectTagListQueryParamsSerializer, @@ -210,6 +210,7 @@ class TaxonomyView(ModelViewSet): lookup_value_regex = r'-?\d+' serializer_class = TaxonomySerializer permission_classes = [TaxonomyObjectPermissions] + pagination_class = TaxonomyPagination def get_object(self) -> Taxonomy: """ diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index c7766a3f..0d880f54 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -66,7 +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 + assert data.get("user_permissions") == user_permissions class TestTaxonomyViewMixin(APITestCase): @@ -152,8 +152,8 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c response = self.client.get(url) assert response.status_code == expected_status - # Check results - if tags_count: + # Check response + if status.is_success(expected_status): assert response.data["results"] == [ { "id": -1, @@ -192,6 +192,12 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c }, }, ] + assert response.data.get("user_permissions") == { + 'can_add': is_admin, + 'can_view': True, + 'can_change': is_admin, + 'can_delete': is_admin, + } def test_list_taxonomy_pagination(self) -> None: url = TAXONOMY_LIST_URL @@ -214,6 +220,40 @@ def test_list_taxonomy_pagination(self) -> None: next_page = parse_qs(parsed_url.query).get("page", [""])[0] assert next_page == "3" + assert response.data.get("user_permissions") == { + 'can_add': True, + 'can_view': True, + 'can_change': True, + 'can_delete': True, + } + + def test_list_taxonomy_empty(self) -> None: + # Delete the language taxonomy so we can get an empty list + language_taxonomy = api.get_taxonomy(LANGUAGE_TAXONOMY_ID) + if language_taxonomy: + language_taxonomy.delete() + + url = TAXONOMY_LIST_URL + self.client.force_authenticate(user=self.user) + response = self.client.get(url, format="json") + + assert response.status_code == status.HTTP_200_OK + assert response.data == { + "count": 0, + "current_page": 1, + "next": None, + "num_pages": 1, + "previous": None, + "start": 0, + "results": [], + "user_permissions": { + 'can_add': False, + 'can_view': True, + 'can_change': False, + 'can_delete': False, + } + } + def test_list_invalid_page(self) -> None: url = TAXONOMY_LIST_URL @@ -248,6 +288,12 @@ def test_language_taxonomy(self): "can_delete": False, }, ) + assert response.data.get("user_permissions") == { + 'can_add': False, + 'can_view': True, + 'can_change': False, + 'can_delete': False, + } @ddt.data( (None, {"enabled": True}, status.HTTP_401_UNAUTHORIZED), @@ -824,8 +870,9 @@ def test_retrieve_object_tags_query_count(self): self.client.force_authenticate(user=self.user_1) with self.assertNumQueries(1): response = self.client.get(url) - assert response.status_code == 200 - assert response.data[object_id]["taxonomies"][0]["tags"] == sort_test_applied_result + + assert response.status_code == 200 + assert response.data[object_id]["taxonomies"][0]["tags"] == sort_test_applied_result def test_retrieve_object_tags_unauthorized(self): """ @@ -1271,6 +1318,14 @@ def test_small_taxonomy_root(self): assert data.get("num_pages") == 1 assert data.get("current_page") == 1 + # Checking models permissions + assert data.get("user_permissions") == { + 'can_add': True, + 'can_view': True, + 'can_change': True, + 'can_delete': True, + } + def test_small_taxonomy(self): """ Test loading all the tags of a small taxonomy at once. @@ -1311,6 +1366,14 @@ def test_small_taxonomy(self): assert data.get("num_pages") == 1 assert data.get("current_page") == 1 + # Checking models permissions + assert data.get("user_permissions") == { + 'can_add': True, + 'can_view': True, + 'can_change': True, + 'can_delete': True, + } + def test_small_taxonomy_paged(self): """ Test loading only the first few of the tags of a small taxonomy. @@ -1341,6 +1404,14 @@ def test_small_taxonomy_paged(self): ] assert next_data.get("current_page") == 2 + # Checking models permissions + assert data.get("user_permissions") == { + 'can_add': True, + 'can_view': True, + 'can_change': True, + 'can_delete': True, + } + def test_small_search(self): """ Test performing a search @@ -1367,6 +1438,14 @@ def test_small_search(self): assert data.get("num_pages") == 1 assert data.get("current_page") == 1 + # Checking models permissions + assert data.get("user_permissions") == { + 'can_add': True, + 'can_view': True, + 'can_change': True, + 'can_delete': True, + } + def test_small_search_shallow(self): """ Test performing a search without full_depth_threshold @@ -1391,6 +1470,14 @@ def test_small_search_shallow(self): assert data.get("num_pages") == 1 assert data.get("current_page") == 1 + # Checking models permissions + assert data.get("user_permissions") == { + 'can_add': True, + 'can_view': True, + 'can_change': True, + 'can_delete': True, + } + # And we can load the sub_tags_url to "drill down" into the search: sub_tags_response = self.client.get(data["results"][0]["sub_tags_url"]) assert sub_tags_response.status_code == status.HTTP_200_OK @@ -1411,6 +1498,12 @@ def assert_empty(url): assert response.status_code == status.HTTP_200_OK assert response.data["results"] == [] assert response.data["count"] == 0 + assert response.data.get("user_permissions") == { + 'can_add': True, + 'can_view': True, + 'can_change': True, + 'can_delete': True, + } # Search terms that won't match any tags: assert_empty(f"{self.small_taxonomy_url}?search_term=foobar") @@ -1470,6 +1563,14 @@ def test_large_taxonomy(self): assert data.get("num_pages") == 6 assert data.get("current_page") == 1 + # Checking models permissions + assert data.get("user_permissions") == { + 'can_add': True, + 'can_view': True, + 'can_change': True, + 'can_delete': True, + } + def test_next_page_large_taxonomy(self): self._build_large_taxonomy() self.client.force_authenticate(user=self.staff) @@ -1496,6 +1597,14 @@ def test_next_page_large_taxonomy(self): assert data.get("num_pages") == 6 assert data.get("current_page") == 2 + # Checking models permissions + assert data.get("user_permissions") == { + 'can_add': True, + 'can_view': True, + 'can_change': True, + 'can_delete': True, + } + def test_large_search(self): """ Test searching in a large taxonomy @@ -1540,6 +1649,12 @@ def test_large_search(self): assert len(results) == expected_num_results assert data.get("num_pages") == 1 assert data.get("current_page") == 1 + assert data.get("user_permissions") == { + 'can_add': True, + 'can_view': True, + 'can_change': True, + 'can_delete': True, + } # Now, perform the search with full_depth_threshold=100, which will give us paginated results, since there are # more than 100 matches: @@ -1564,6 +1679,12 @@ def test_large_search(self): assert data2.get("count") == 51 assert data2.get("num_pages") == 6 assert data2.get("current_page") == 1 + assert data2.get("user_permissions") == { + 'can_add': True, + 'can_view': True, + 'can_change': True, + 'can_delete': True, + } # Now load the results that are in the subtree of the root tag 'Tag 0' tag_0_subtags_url = results2[0]["sub_tags_url"] @@ -1586,6 +1707,12 @@ def test_large_search(self): " Tag 118 (Tag 0) (children: 1)", " Tag 119 (Tag 118) (children: 0)", ] + assert data3.get("user_permissions") == { + 'can_add': True, + 'can_view': True, + 'can_change': True, + 'can_delete': True, + } def test_get_children(self): self._build_large_taxonomy() @@ -1627,6 +1754,14 @@ def test_get_children(self): assert data.get("num_pages") == 2 assert data.get("current_page") == 1 + # Checking model permissions + assert data.get("user_permissions") == { + 'can_add': True, + 'can_view': True, + 'can_change': True, + 'can_delete': True, + } + def test_get_leaves(self): """ Test getting the tags at depth=2, using "full_depth_threshold=1000" to @@ -1654,6 +1789,14 @@ def test_get_leaves(self): ] assert response.data.get("next") is None + # Checking model permissions + assert response.data.get("user_permissions") == { + 'can_add': True, + 'can_view': True, + 'can_change': True, + 'can_delete': True, + } + def test_get_leaves_paginated(self): """ Test getting depth=2 entries, disabling the feature to return the whole @@ -1678,12 +1821,25 @@ def test_get_leaves_paginated(self): ] next_url = response.data.get("next") assert next_url is not None + assert response.data.get("user_permissions") == { + 'can_add': True, + 'can_view': True, + 'can_change': True, + 'can_delete': True, + } + response2 = self.client.get(next_url) results2 = response2.data["results"] assert pretty_format_tags(results2) == [ " Placozoa (Animalia) (children: 0)", " Porifera (Animalia) (children: 0)", ] + assert response2.data.get("user_permissions") == { + 'can_add': True, + 'can_view': True, + 'can_change': True, + 'can_delete': True, + } def test_next_children(self): self._build_large_taxonomy() @@ -1719,6 +1875,14 @@ def test_next_children(self): assert data.get("num_pages") == 2 assert data.get("current_page") == 2 + # Checking model permissions + assert data.get("user_permissions") == { + 'can_add': True, + 'can_view': True, + 'can_change': True, + 'can_delete': True, + } + def test_create_tag_in_taxonomy_while_loggedout(self): new_tag_value = "New Tag" From 634168a67628ce07be97d92667973959eb202dcf Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 10 Jan 2024 16:56:32 +1030 Subject: [PATCH 06/17] fix: return only the permissions we need * model-level permissions: only need can_add * instance-level permissions: only need can_change and can_delete --- .../core/tagging/rest_api/paginators.py | 30 ++- .../core/tagging/rest_api/v1/serializers.py | 19 +- .../core/tagging/test_views.py | 195 +++--------------- 3 files changed, 45 insertions(+), 199 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/paginators.py b/openedx_tagging/core/tagging/rest_api/paginators.py index abcd5ba6..dfba0f1a 100644 --- a/openedx_tagging/core/tagging/rest_api/paginators.py +++ b/openedx_tagging/core/tagging/rest_api/paginators.py @@ -12,12 +12,12 @@ MAX_FULL_DEPTH_THRESHOLD = 10_000 -class ModelPermissionsPaginationMixin: +class CanAddPermissionMixin: """ - Inserts model permissions data into the top level of the paginated response. + Inserts a field into the top level of the paginated response indicating whether the request user has permission to + add new instances of the current model. """ - permission_field_name = 'user_permissions' - permission_actions = ('add', 'view', 'change', 'delete') + field_name = 'can_add' def get_model(self) -> Type: """ @@ -25,34 +25,32 @@ def get_model(self) -> Type: """ raise NotImplementedError # pragma: no cover - def get_model_permissions(self) -> dict: + def get_can_add(self) -> bool: """ - Returns a dict containing the request user's permissions for the current model. - - The dict contains keys named `can_`, mapped to a boolean flag. + Returns True if the current user can add models. """ user = self.request.user # type: ignore[attr-defined] model = self.get_model() app_label = model._meta.app_label model_name = model._meta.model_name - return { - f'can_{action}': user.has_perm(f'{app_label}.{action}_{model_name}') - for action in self.permission_actions - } + return user.has_perm(f'{app_label}.add_{model_name}') def get_paginated_response(self, data) -> Response: """ Injects the user's model-level permissions into the paginated response. """ response_data = super().get_paginated_response(data).data # type: ignore[misc] - response_data[self.permission_field_name] = self.get_model_permissions() + response_data[self.field_name] = self.get_can_add() return Response(response_data) -class TaxonomyPagination(ModelPermissionsPaginationMixin, DefaultPagination): +class TaxonomyPagination(CanAddPermissionMixin, DefaultPagination): """ Inserts permissions data for Taxonomies into the top level of the paginated response. """ + page_size = 500 + max_page_size = 500 + def get_model(self) -> Type: """ Returns the model that is being paginated. @@ -60,7 +58,7 @@ def get_model(self) -> Type: return Taxonomy -class TagsPagination(ModelPermissionsPaginationMixin, DefaultPagination): +class TagsPagination(CanAddPermissionMixin, DefaultPagination): """ Custom pagination configuration for taxonomies with a large number of tags. Used on the get tags API view. @@ -75,7 +73,7 @@ def get_model(self) -> Type: return Tag -class DisabledTagsPagination(ModelPermissionsPaginationMixin, DefaultPagination): +class DisabledTagsPagination(CanAddPermissionMixin, DefaultPagination): """ Custom pagination configuration for taxonomies with a small number of tags. Used on the get tags API view diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index a393e956..36c32e88 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -36,8 +36,6 @@ class UserPermissionsSerializer(serializers.Serializer): # pylint: disable=abst 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() @@ -47,8 +45,9 @@ def _check_permission(self, action, instance) -> bool: Uses the current request as passed into the serializer context. """ - assert action in ("add", "change", "view", "delete") + assert action in ("change", "delete") + # We can't just use the instance._meta.model_name, because of subclasses. if isinstance(instance, Taxonomy): model = 'taxonomy' elif isinstance(instance, Tag): @@ -61,29 +60,15 @@ def _check_permission(self, action, instance) -> bool: request = self.context.get('request') assert request and request.user - if action == 'add': - instance = None 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. diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 0d880f54..6e15087a 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -167,8 +167,6 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c "tags_count": 0, # System taxonomy cannot be modified "user_permissions": { - "can_add": is_admin, - "can_view": True, "can_change": False, "can_delete": False, }, @@ -185,19 +183,12 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c "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, }, }, ] - assert response.data.get("user_permissions") == { - 'can_add': is_admin, - 'can_view': True, - 'can_change': is_admin, - 'can_delete': is_admin, - } + assert response.data.get("can_add") == is_admin def test_list_taxonomy_pagination(self) -> None: url = TAXONOMY_LIST_URL @@ -220,21 +211,22 @@ def test_list_taxonomy_pagination(self) -> None: next_page = parse_qs(parsed_url.query).get("page", [""])[0] assert next_page == "3" - assert response.data.get("user_permissions") == { - 'can_add': True, - 'can_view': True, - 'can_change': True, - 'can_delete': True, - } + assert response.data.get("can_add") - def test_list_taxonomy_empty(self) -> None: + @ddt.data( + ('user', False), + ('staff', True), + ) + @ddt.unpack + def test_list_taxonomy_empty(self, user_attr, expected_can_add) -> None: # Delete the language taxonomy so we can get an empty list language_taxonomy = api.get_taxonomy(LANGUAGE_TAXONOMY_ID) if language_taxonomy: language_taxonomy.delete() url = TAXONOMY_LIST_URL - self.client.force_authenticate(user=self.user) + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) response = self.client.get(url, format="json") assert response.status_code == status.HTTP_200_OK @@ -246,12 +238,7 @@ def test_list_taxonomy_empty(self) -> None: "previous": None, "start": 0, "results": [], - "user_permissions": { - 'can_add': False, - 'can_view': True, - 'can_change': False, - 'can_delete': False, - } + "can_add": expected_can_add, } def test_list_invalid_page(self) -> None: @@ -282,18 +269,11 @@ def test_language_taxonomy(self): 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, }, ) - assert response.data.get("user_permissions") == { - 'can_add': False, - 'can_view': True, - 'can_change': False, - 'can_delete': False, - } + assert not response.data.get("can_add") @ddt.data( (None, {"enabled": True}, status.HTTP_401_UNAUTHORIZED), @@ -321,8 +301,6 @@ def test_detail_taxonomy( if status.is_success(expected_status): expected_data = create_data expected_data["user_permissions"] = { - "can_add": is_admin, - "can_view": True, "can_change": is_admin, "can_delete": is_admin, } @@ -376,8 +354,6 @@ def test_create_taxonomy(self, user_attr: str | None, expected_status: int): if status.is_success(expected_status): expected_data = create_data expected_data["user_permissions"] = { - "can_add": True, - "can_view": True, "can_change": True, "can_delete": True, } @@ -443,8 +419,6 @@ def test_update_taxonomy(self, user_attr, expected_status): "description": "taxonomy description", "enabled": True, "user_permissions": { - "can_add": True, - "can_view": True, "can_change": True, "can_delete": True, }, @@ -505,8 +479,6 @@ 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, }, @@ -743,8 +715,6 @@ 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, }, @@ -753,8 +723,6 @@ def test_retrieve_object_tags(self, user_attr, expected_status): "value": "Fungi", "lineage": ["Eukaryota", "Fungi"], "user_permissions": { - "can_add": True, - "can_view": True, "can_change": True, "can_delete": True, }, @@ -769,8 +737,6 @@ 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, }, @@ -801,8 +767,6 @@ def prepare_for_sort_test(self) -> tuple[str, list[dict]]: shared_props = { "user_permissions": { - "can_add": True, - "can_view": True, "can_change": True, "can_delete": True, }, @@ -924,8 +888,6 @@ 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, }, @@ -1317,14 +1279,7 @@ def test_small_taxonomy_root(self): assert data.get("count") == root_count assert data.get("num_pages") == 1 assert data.get("current_page") == 1 - - # Checking models permissions - assert data.get("user_permissions") == { - 'can_add': True, - 'can_view': True, - 'can_change': True, - 'can_delete': True, - } + assert data.get("can_add") def test_small_taxonomy(self): """ @@ -1365,14 +1320,7 @@ def test_small_taxonomy(self): assert data.get("count") == len(results) assert data.get("num_pages") == 1 assert data.get("current_page") == 1 - - # Checking models permissions - assert data.get("user_permissions") == { - 'can_add': True, - 'can_view': True, - 'can_change': True, - 'can_delete': True, - } + assert data.get("can_add") def test_small_taxonomy_paged(self): """ @@ -1394,6 +1342,7 @@ def test_small_taxonomy_paged(self): assert data.get("count") == 3 assert data.get("num_pages") == 2 assert data.get("current_page") == 1 + assert data.get("can_add") # Get the next page: next_response = self.client.get(data.get("next")) @@ -1403,14 +1352,7 @@ def test_small_taxonomy_paged(self): "Eukaryota (None) (children: 5)", ] assert next_data.get("current_page") == 2 - - # Checking models permissions - assert data.get("user_permissions") == { - 'can_add': True, - 'can_view': True, - 'can_change': True, - 'can_delete': True, - } + assert next_data.get("can_add") def test_small_search(self): """ @@ -1437,14 +1379,7 @@ def test_small_search(self): assert data.get("count") == 5 assert data.get("num_pages") == 1 assert data.get("current_page") == 1 - - # Checking models permissions - assert data.get("user_permissions") == { - 'can_add': True, - 'can_view': True, - 'can_change': True, - 'can_delete': True, - } + assert data.get("can_add") def test_small_search_shallow(self): """ @@ -1469,14 +1404,7 @@ def test_small_search_shallow(self): assert data.get("count") == 3 assert data.get("num_pages") == 1 assert data.get("current_page") == 1 - - # Checking models permissions - assert data.get("user_permissions") == { - 'can_add': True, - 'can_view': True, - 'can_change': True, - 'can_delete': True, - } + assert data.get("can_add") # And we can load the sub_tags_url to "drill down" into the search: sub_tags_response = self.client.get(data["results"][0]["sub_tags_url"]) @@ -1498,12 +1426,7 @@ def assert_empty(url): assert response.status_code == status.HTTP_200_OK assert response.data["results"] == [] assert response.data["count"] == 0 - assert response.data.get("user_permissions") == { - 'can_add': True, - 'can_view': True, - 'can_change': True, - 'can_delete': True, - } + assert response.data.get("can_add") # Search terms that won't match any tags: assert_empty(f"{self.small_taxonomy_url}?search_term=foobar") @@ -1562,14 +1485,7 @@ def test_large_taxonomy(self): assert data.get("count") == self.root_tags_count assert data.get("num_pages") == 6 assert data.get("current_page") == 1 - - # Checking models permissions - assert data.get("user_permissions") == { - 'can_add': True, - 'can_view': True, - 'can_change': True, - 'can_delete': True, - } + assert data.get("can_add") def test_next_page_large_taxonomy(self): self._build_large_taxonomy() @@ -1596,14 +1512,7 @@ def test_next_page_large_taxonomy(self): assert data.get("count") == self.root_tags_count assert data.get("num_pages") == 6 assert data.get("current_page") == 2 - - # Checking models permissions - assert data.get("user_permissions") == { - 'can_add': True, - 'can_view': True, - 'can_change': True, - 'can_delete': True, - } + assert data.get("can_add") def test_large_search(self): """ @@ -1649,12 +1558,7 @@ def test_large_search(self): assert len(results) == expected_num_results assert data.get("num_pages") == 1 assert data.get("current_page") == 1 - assert data.get("user_permissions") == { - 'can_add': True, - 'can_view': True, - 'can_change': True, - 'can_delete': True, - } + assert data.get("can_add") # Now, perform the search with full_depth_threshold=100, which will give us paginated results, since there are # more than 100 matches: @@ -1679,12 +1583,7 @@ def test_large_search(self): assert data2.get("count") == 51 assert data2.get("num_pages") == 6 assert data2.get("current_page") == 1 - assert data2.get("user_permissions") == { - 'can_add': True, - 'can_view': True, - 'can_change': True, - 'can_delete': True, - } + assert data2.get("can_add") # Now load the results that are in the subtree of the root tag 'Tag 0' tag_0_subtags_url = results2[0]["sub_tags_url"] @@ -1707,12 +1606,7 @@ def test_large_search(self): " Tag 118 (Tag 0) (children: 1)", " Tag 119 (Tag 118) (children: 0)", ] - assert data3.get("user_permissions") == { - 'can_add': True, - 'can_view': True, - 'can_change': True, - 'can_delete': True, - } + assert data2.get("can_add") def test_get_children(self): self._build_large_taxonomy() @@ -1753,14 +1647,7 @@ def test_get_children(self): assert data.get("count") == self.children_tags_count[0] assert data.get("num_pages") == 2 assert data.get("current_page") == 1 - - # Checking model permissions - assert data.get("user_permissions") == { - 'can_add': True, - 'can_view': True, - 'can_change': True, - 'can_delete': True, - } + assert data.get("can_add") def test_get_leaves(self): """ @@ -1788,14 +1675,7 @@ def test_get_leaves(self): " Porifera (Animalia) (children: 0)", ] assert response.data.get("next") is None - - # Checking model permissions - assert response.data.get("user_permissions") == { - 'can_add': True, - 'can_view': True, - 'can_change': True, - 'can_delete': True, - } + assert response.data.get("can_add") def test_get_leaves_paginated(self): """ @@ -1821,12 +1701,7 @@ def test_get_leaves_paginated(self): ] next_url = response.data.get("next") assert next_url is not None - assert response.data.get("user_permissions") == { - 'can_add': True, - 'can_view': True, - 'can_change': True, - 'can_delete': True, - } + assert response.data.get("can_add") response2 = self.client.get(next_url) results2 = response2.data["results"] @@ -1834,12 +1709,7 @@ def test_get_leaves_paginated(self): " Placozoa (Animalia) (children: 0)", " Porifera (Animalia) (children: 0)", ] - assert response2.data.get("user_permissions") == { - 'can_add': True, - 'can_view': True, - 'can_change': True, - 'can_delete': True, - } + assert response2.data.get("can_add") def test_next_children(self): self._build_large_taxonomy() @@ -1874,14 +1744,7 @@ def test_next_children(self): assert data.get("count") == self.children_tags_count[0] assert data.get("num_pages") == 2 assert data.get("current_page") == 2 - - # Checking model permissions - assert data.get("user_permissions") == { - 'can_add': True, - 'can_view': True, - 'can_change': True, - 'can_delete': True, - } + assert data.get("can_add") def test_create_tag_in_taxonomy_while_loggedout(self): new_tag_value = "New Tag" From c803f66e662bc1e6b40ffa2f21d01c4a808ddd34 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 11 Jan 2024 15:42:54 +1030 Subject: [PATCH 07/17] refactor: user permissions serializer and paginator share helper class Also flattens the serialized user permissions so the flags appear directly on the instance instead of in a "user_permissions" dict. --- .../core/tagging/rest_api/paginators.py | 36 +++--- .../core/tagging/rest_api/utils.py | 87 +++++++++++++ .../core/tagging/rest_api/v1/serializers.py | 119 +++++++++--------- .../core/tagging/rest_api/v1/utils.py | 24 ---- .../core/tagging/rest_api/v1/views.py | 2 +- .../core/tagging/test_views.py | 78 +++++------- 6 files changed, 191 insertions(+), 155 deletions(-) create mode 100644 openedx_tagging/core/tagging/rest_api/utils.py delete mode 100644 openedx_tagging/core/tagging/rest_api/v1/utils.py diff --git a/openedx_tagging/core/tagging/rest_api/paginators.py b/openedx_tagging/core/tagging/rest_api/paginators.py index dfba0f1a..f5d37816 100644 --- a/openedx_tagging/core/tagging/rest_api/paginators.py +++ b/openedx_tagging/core/tagging/rest_api/paginators.py @@ -4,36 +4,31 @@ from typing import Type from edx_rest_framework_extensions.paginators import DefaultPagination # type: ignore[import] +from rest_framework.request import Request from rest_framework.response import Response from openedx_tagging.core.tagging.models import Tag, Taxonomy +from .utils import UserPermissionsHelper + # From this point, the tags begin to be paginated MAX_FULL_DEPTH_THRESHOLD = 10_000 -class CanAddPermissionMixin: +class CanAddPermissionMixin(UserPermissionsHelper): # pylint: disable=abstract-method """ - Inserts a field into the top level of the paginated response indicating whether the request user has permission to - add new instances of the current model. + This mixin inserts a boolean "can_add" field at the top level of the paginated response. + + The value of the field indicates whether request user may create new instances of the current model. """ field_name = 'can_add' - def get_model(self) -> Type: - """ - Returns the model that is being paginated. - """ - raise NotImplementedError # pragma: no cover - - def get_can_add(self) -> bool: + @property + def _request(self) -> Request: """ - Returns True if the current user can add models. + Returns the current request. """ - user = self.request.user # type: ignore[attr-defined] - model = self.get_model() - app_label = model._meta.app_label - model_name = model._meta.model_name - return user.has_perm(f'{app_label}.add_{model_name}') + return self.request # type: ignore[attr-defined] def get_paginated_response(self, data) -> Response: """ @@ -51,7 +46,8 @@ class TaxonomyPagination(CanAddPermissionMixin, DefaultPagination): page_size = 500 max_page_size = 500 - def get_model(self) -> Type: + @property + def _model(self) -> Type: """ Returns the model that is being paginated. """ @@ -66,7 +62,8 @@ class TagsPagination(CanAddPermissionMixin, DefaultPagination): page_size = 10 max_page_size = 300 - def get_model(self) -> Type: + @property + def _model(self) -> Type: """ Returns the model that is being paginated. """ @@ -85,7 +82,8 @@ class DisabledTagsPagination(CanAddPermissionMixin, DefaultPagination): page_size = MAX_FULL_DEPTH_THRESHOLD max_page_size = MAX_FULL_DEPTH_THRESHOLD + 1 - def get_model(self) -> Type: + @property + def _model(self) -> Type: """ Returns the model that is being paginated. """ diff --git a/openedx_tagging/core/tagging/rest_api/utils.py b/openedx_tagging/core/tagging/rest_api/utils.py new file mode 100644 index 00000000..ddf10f30 --- /dev/null +++ b/openedx_tagging/core/tagging/rest_api/utils.py @@ -0,0 +1,87 @@ +""" +Utilities for the API +""" +from typing import Type + +from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication # type: ignore[import] +from edx_rest_framework_extensions.auth.session.authentication import ( # type: ignore[import] + SessionAuthenticationAllowInactiveUser, +) +from rest_framework.request import Request + + +def view_auth_classes(func_or_class): + """ + Function and class decorator that abstracts the authentication classes for api views. + """ + def _decorator(func_or_class): + """ + Requires either OAuth2 or Session-based authentication; + are the same authentication classes used on edx-platform + """ + func_or_class.authentication_classes = ( + JwtAuthentication, + SessionAuthenticationAllowInactiveUser, + ) + return func_or_class + return _decorator(func_or_class) + + +class UserPermissionsHelper: + """ + Provides helper methods for serializing user permissions. + """ + @property + def _model(self) -> Type: + """ + Returns the model used when checking permissions. + """ + raise NotImplementedError # pragma: no cover + + @property + def _request(self) -> Request: + """ + Returns the current request. + """ + raise NotImplementedError # pragma: no cover + + def _can(self, action: str, instance=None) -> bool: + """ + Returns True if the current `request.user` may perform the given `action` on the `instance` object. + """ + assert action in ("add", "view", "change", "delete") + request = self._request + assert request and request.user + model = self._model + assert model + + app_label = model._meta.app_label + model_name = model._meta.model_name + perm_name = f'{app_label}.{action}_{model_name}' + return request.user.has_perm(perm_name, instance) + + def get_can_add(self, _instance=None) -> bool: + """ + Returns True if the current user is allowed to add new instances. + + Note: we omit the actual instance from the permissions check; most tagging models prefer this. + """ + return self._can('add') + + def get_can_view(self, instance) -> bool: + """ + Returns True if the current user is allowed to view/see this instance. + """ + return self._can('view', instance) + + def get_can_change(self, instance) -> bool: + """ + Returns True if the current user is allowed to edit/change this instance. + """ + return self._can('change', instance) + + def get_can_delete(self, instance) -> bool: + """ + Returns True if the current user is allowed to delete this instance. + """ + return self._can('delete', instance) diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index 36c32e88..69a76ee9 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -3,16 +3,19 @@ """ from __future__ import annotations -from typing import Any +from typing import Any, Type from urllib.parse import urlencode from rest_framework import serializers +from rest_framework.request import Request from rest_framework.reverse import reverse from openedx_tagging.core.tagging.data import TagData from openedx_tagging.core.tagging.import_export.parsers import ParserFormat from openedx_tagging.core.tagging.models import ObjectTag, Tag, TagImportTask, Taxonomy +from ..utils import UserPermissionsHelper + class TaxonomyListQueryParamsSerializer(serializers.Serializer): # pylint: disable=abstract-method """ @@ -30,58 +33,43 @@ 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. +class UserPermissionsSerializerMixin(UserPermissionsHelper): """ - 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 ("change", "delete") + Provides methods for serializing user permissions. - # We can't just use the instance._meta.model_name, because of subclasses. - if isinstance(instance, Taxonomy): - model = 'taxonomy' - elif isinstance(instance, Tag): - model = 'tag' - elif isinstance(instance, ObjectTag): - model = 'objecttag' - else: - return False + To use this mixin: - request = self.context.get('request') - assert request and request.user + 1. Add it to your serializer's list of subclasses + 2. Add `can_` fields for each permission/action you want to serialize. - permission = f"oel_tagging.{action}_{model}" - return request.user.has_perm(permission, instance) + and this mixin will take care of the rest. - def get_can_change(self, instance) -> bool: + Notes: + * Assumes the serialized model should be used to check permissions (override _model to change). + * Requires the current request to be passed into the serializer context (override _request to change). + """ + @property + def _model(self) -> Type: """ - Returns True if the current user is allowed to change this instance. + Returns the model that is being serialized """ - return self._check_permission('change', instance) + return self.Meta.model # type: ignore[attr-defined] - def get_can_delete(self, instance) -> bool: + @property + def _request(self) -> Request: """ - Returns True if the current user is allowed to delete this instance. + Returns the current request from the serialize context. """ - return self._check_permission('delete', instance) + return self.context.get('request') # type: ignore[attr-defined] -class TaxonomySerializer(serializers.ModelSerializer): +class TaxonomySerializer(UserPermissionsSerializerMixin, serializers.ModelSerializer): """ Serializer for the Taxonomy model. """ tags_count = serializers.SerializerMethodField() - user_permissions = serializers.SerializerMethodField() + can_change = serializers.SerializerMethodField() + can_delete = serializers.SerializerMethodField() class Meta: model = Taxonomy @@ -95,7 +83,8 @@ class Meta: "system_defined", "visible_to_authors", "tags_count", - "user_permissions", + "can_change", + "can_delete", ] def to_representation(self, instance): @@ -108,13 +97,6 @@ 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 """ @@ -126,24 +108,18 @@ class ObjectTagListQueryParamsSerializer(serializers.Serializer): # pylint: dis ) -class ObjectTagMinimalSerializer(serializers.ModelSerializer): +class ObjectTagMinimalSerializer(UserPermissionsSerializerMixin, serializers.ModelSerializer): """ Minimal serializer for the ObjectTag model. """ class Meta: model = ObjectTag - fields = ["value", "lineage", "user_permissions"] + fields = ["value", "lineage", "can_change", "can_delete"] 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) + can_change = serializers.SerializerMethodField() + can_delete = serializers.SerializerMethodField() class ObjectTagSerializer(ObjectTagMinimalSerializer): @@ -206,7 +182,7 @@ class ObjectTagUpdateQueryParamsSerializer(serializers.Serializer): # pylint: d ) -class TagDataSerializer(serializers.Serializer): # pylint: disable=abstract-method +class TagDataSerializer(UserPermissionsSerializerMixin, serializers.Serializer): # pylint: disable=abstract-method """ Serializer for TagData dicts. Also can serialize Tag instances. @@ -223,7 +199,8 @@ class TagDataSerializer(serializers.Serializer): # pylint: disable=abstract-met _id = serializers.IntegerField(allow_null=True) sub_tags_url = serializers.SerializerMethodField() - user_permissions = serializers.SerializerMethodField() + can_change = serializers.SerializerMethodField() + can_delete = serializers.SerializerMethodField() def get_sub_tags_url(self, obj: TagData | Tag): """ @@ -247,12 +224,32 @@ def get_sub_tags_url(self, obj: TagData | Tag): return request.build_absolute_uri(url) return None - def get_user_permissions(self, instance): + @property + def _model(self) -> Type: + """ + Returns the model used when checking permissions. + """ + return Tag + + def get_can_change(self, instance) -> bool: """ - Returns the serialized user permissions on the given instance. + Returns True if the current user is allowed to edit/change this Tag instance. + + Returns False for all TagData instances. """ - permissions = UserPermissionsSerializer(instance, context=self.context) - return permissions.to_representation(instance) + if isinstance(instance, Tag): + return super()._can('change', instance) + return False + + def get_can_delete(self, instance) -> bool: + """ + Returns True if the current user is allowed to delete this Tag instance. + + Returns False for all TagData instances. + """ + if isinstance(instance, Tag): + return super()._can('delete', instance) + return False def to_representation(self, instance: TagData | Tag) -> dict: """ diff --git a/openedx_tagging/core/tagging/rest_api/v1/utils.py b/openedx_tagging/core/tagging/rest_api/v1/utils.py deleted file mode 100644 index 0667fb3b..00000000 --- a/openedx_tagging/core/tagging/rest_api/v1/utils.py +++ /dev/null @@ -1,24 +0,0 @@ -""" -Utilities for the API -""" -from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication # type: ignore[import] -from edx_rest_framework_extensions.auth.session.authentication import ( # type: ignore[import] - SessionAuthenticationAllowInactiveUser, -) - - -def view_auth_classes(func_or_class): - """ - Function and class decorator that abstracts the authentication classes for api views. - """ - def _decorator(func_or_class): - """ - Requires either OAuth2 or Session-based authentication; - are the same authentication classes used on edx-platform - """ - func_or_class.authentication_classes = ( - JwtAuthentication, - SessionAuthenticationAllowInactiveUser, - ) - return func_or_class - return _decorator(func_or_class) diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 42192bc6..7e21974f 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -31,6 +31,7 @@ from ...models import Taxonomy from ...rules import ObjectTagPermissionItem from ..paginators import MAX_FULL_DEPTH_THRESHOLD, DisabledTagsPagination, TagsPagination, TaxonomyPagination +from ..utils import view_auth_classes from .permissions import ObjectTagObjectPermissions, TagObjectPermissions, TaxonomyObjectPermissions from .serializers import ( ObjectTagListQueryParamsSerializer, @@ -49,7 +50,6 @@ TaxonomyTagDeleteBodySerializer, TaxonomyTagUpdateBodySerializer, ) -from .utils import view_auth_classes @view_auth_classes diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 6e15087a..f6e98d96 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -53,7 +53,8 @@ def check_taxonomy( allow_free_text=False, system_defined=False, visible_to_authors=True, - user_permissions=None, + can_change=False, + can_delete=False, ): """ Check taxonomy data @@ -66,7 +67,8 @@ 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.get("user_permissions") == user_permissions + assert data["can_change"] == can_change + assert data["can_delete"] == can_delete class TestTaxonomyViewMixin(APITestCase): @@ -166,10 +168,8 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c "visible_to_authors": True, "tags_count": 0, # System taxonomy cannot be modified - "user_permissions": { - "can_change": False, - "can_delete": False, - }, + "can_change": False, + "can_delete": False, }, { "id": taxonomy.id, @@ -182,10 +182,8 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c "visible_to_authors": True, "tags_count": tags_count, # Enabled taxonomy can be modified by staff - "user_permissions": { - "can_change": is_admin, - "can_delete": is_admin, - }, + "can_change": is_admin, + "can_delete": is_admin, }, ] assert response.data.get("can_add") == is_admin @@ -268,10 +266,8 @@ 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_change": False, - "can_delete": False, - }, + can_change=False, + can_delete=False, ) assert not response.data.get("can_add") @@ -300,10 +296,8 @@ def test_detail_taxonomy( if status.is_success(expected_status): expected_data = create_data - expected_data["user_permissions"] = { - "can_change": is_admin, - "can_delete": is_admin, - } + expected_data["can_change"] = is_admin + expected_data["can_delete"] = is_admin check_taxonomy(response.data, taxonomy.pk, **expected_data) def test_detail_system_taxonomy(self): @@ -353,10 +347,8 @@ 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): expected_data = create_data - expected_data["user_permissions"] = { - "can_change": True, - "can_delete": True, - } + expected_data["can_change"] = True + expected_data["can_delete"] = True check_taxonomy(response.data, response.data["id"], **expected_data) url = TAXONOMY_DETAIL_URL.format(pk=response.data["id"]) @@ -418,10 +410,8 @@ def test_update_taxonomy(self, user_attr, expected_status): "name": "new name", "description": "taxonomy description", "enabled": True, - "user_permissions": { - "can_change": True, - "can_delete": True, - }, + "can_change": True, + "can_delete": True, }, ) @@ -478,10 +468,8 @@ def test_patch_taxonomy(self, user_attr, expected_status): **{ "name": "new name", "enabled": True, - "user_permissions": { - "can_change": True, - "can_delete": True, - }, + "can_change": True, + "can_delete": True, }, ) @@ -714,18 +702,14 @@ def test_retrieve_object_tags(self, user_attr, expected_status): { "value": "Mammalia", "lineage": ["Eukaryota", "Animalia", "Chordata", "Mammalia"], - "user_permissions": { - "can_change": True, - "can_delete": True, - }, + "can_change": True, + "can_delete": True, }, { "value": "Fungi", "lineage": ["Eukaryota", "Fungi"], - "user_permissions": { - "can_change": True, - "can_delete": True, - }, + "can_change": True, + "can_delete": True, }, ] }, @@ -736,10 +720,8 @@ def test_retrieve_object_tags(self, user_attr, expected_status): { "value": "test_user_1", "lineage": ["test_user_1"], - "user_permissions": { - "can_change": True, - "can_delete": True, - }, + "can_change": True, + "can_delete": True, }, ], } @@ -766,10 +748,8 @@ def prepare_for_sort_test(self) -> tuple[str, list[dict]]: ] shared_props = { - "user_permissions": { - "can_change": True, - "can_delete": True, - }, + "can_change": True, + "can_delete": True, } # Apply the object tags: @@ -887,10 +867,8 @@ def test_retrieve_object_tags_taxonomy_queryparam( { "value": "test_user_1", "lineage": ["test_user_1"], - "user_permissions": { - "can_change": True, - "can_delete": True, - }, + "can_change": True, + "can_delete": True, }, ], } From 2709d44992c4eb5197bd3b5ec39f765c6b843e26 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 11 Jan 2024 22:36:40 +1030 Subject: [PATCH 08/17] feat: adds can_tag_object field to ObjectTagsByTaxonomySerializer to tell REST API users whether the user may add object tags for this object_id + taxonomy. --- openedx_tagging/core/tagging/rest_api/v1/serializers.py | 6 +++++- tests/openedx_tagging/core/tagging/test_views.py | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index 69a76ee9..7f498ea3 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -137,10 +137,13 @@ class Meta: ] -class ObjectTagsByTaxonomySerializer(serializers.ModelSerializer): +class ObjectTagsByTaxonomySerializer(UserPermissionsSerializerMixin, serializers.ModelSerializer): """ Serialize a group of ObjectTags, grouped by taxonomy """ + class Meta: + model = ObjectTag + def to_representation(self, instance: list[ObjectTag]) -> dict: """ Convert this list of ObjectTags to the serialized dictionary, grouped by Taxonomy @@ -157,6 +160,7 @@ def to_representation(self, instance: list[ObjectTag]) -> dict: tax_entry = { "name": obj_tag.name, "taxonomy_id": obj_tag.taxonomy_id, + "can_tag_object": self._can('add', obj_tag), "tags": [] } taxonomies.append(tax_entry) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index f6e98d96..04e3e09a 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -696,6 +696,7 @@ def test_retrieve_object_tags(self, user_attr, expected_status): { "name": "Life on Earth", "taxonomy_id": 1, + "can_tag_object": True, "tags": [ # Note: based on tree order (Animalia before Fungi), this tag comes first even though it # starts with "M" and Fungi starts with "F" @@ -716,6 +717,7 @@ def test_retrieve_object_tags(self, user_attr, expected_status): { "name": "User Authors", "taxonomy_id": 3, + "can_tag_object": True, "tags": [ { "value": "test_user_1", @@ -863,6 +865,7 @@ def test_retrieve_object_tags_taxonomy_queryparam( { "name": "User Authors", "taxonomy_id": 3, + "can_tag_object": None, "tags": [ { "value": "test_user_1", From 747090479264531f055357bd49a8b35a862b21ab Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 11 Jan 2024 22:41:31 +1030 Subject: [PATCH 09/17] feat: check permissions only if ?include_perms * `can_` field will be None unless ?include_perms requested * adds tests to verify query counts are unchanged when perms requested --- .../core/tagging/rest_api/utils.py | 34 +++- .../core/tagging/rest_api/v1/serializers.py | 12 +- .../core/tagging/rest_api/v1/views.py | 43 ++++- .../core/tagging/test_views.py | 149 ++++++++++++------ 4 files changed, 174 insertions(+), 64 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/utils.py b/openedx_tagging/core/tagging/rest_api/utils.py index ddf10f30..9e7fa0a7 100644 --- a/openedx_tagging/core/tagging/rest_api/utils.py +++ b/openedx_tagging/core/tagging/rest_api/utils.py @@ -1,7 +1,7 @@ """ Utilities for the API """ -from typing import Type +from typing import Optional, Type from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication # type: ignore[import] from edx_rest_framework_extensions.auth.session.authentication import ( # type: ignore[import] @@ -45,13 +45,31 @@ def _request(self) -> Request: """ raise NotImplementedError # pragma: no cover - def _can(self, action: str, instance=None) -> bool: + @property + def _include_perms(self) -> bool: """ - Returns True if the current `request.user` may perform the given `action` on the `instance` object. + Are permission checks requested? + + Returns True if ?include_perms found in the query string + Returns False by default. + """ + return 'include_perms' in self._request.query_params + + def _can(self, action: str, instance=None) -> Optional[bool]: + """ + Can the current `request.user` perform the given `action` on the `instance` object? + + Returns None if no permissions were requested. + Returns True if they may. + Returns False if they may not. """ - assert action in ("add", "view", "change", "delete") request = self._request assert request and request.user + if not self._include_perms: + return None + + assert action in ("add", "view", "change", "delete") + model = self._model assert model @@ -60,7 +78,7 @@ def _can(self, action: str, instance=None) -> bool: perm_name = f'{app_label}.{action}_{model_name}' return request.user.has_perm(perm_name, instance) - def get_can_add(self, _instance=None) -> bool: + def get_can_add(self, _instance=None) -> Optional[bool]: """ Returns True if the current user is allowed to add new instances. @@ -68,19 +86,19 @@ def get_can_add(self, _instance=None) -> bool: """ return self._can('add') - def get_can_view(self, instance) -> bool: + def get_can_view(self, instance) -> Optional[bool]: """ Returns True if the current user is allowed to view/see this instance. """ return self._can('view', instance) - def get_can_change(self, instance) -> bool: + def get_can_change(self, instance) -> Optional[bool]: """ Returns True if the current user is allowed to edit/change this instance. """ return self._can('change', instance) - def get_can_delete(self, instance) -> bool: + def get_can_delete(self, instance) -> Optional[bool]: """ Returns True if the current user is allowed to delete this instance. """ diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index 7f498ea3..22d2ec33 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -3,7 +3,7 @@ """ from __future__ import annotations -from typing import Any, Type +from typing import Any, Optional, Type from urllib.parse import urlencode from rest_framework import serializers @@ -47,6 +47,8 @@ class UserPermissionsSerializerMixin(UserPermissionsHelper): Notes: * Assumes the serialized model should be used to check permissions (override _model to change). * Requires the current request to be passed into the serializer context (override _request to change). + * Requires '?include_perms` in the request, otherwise no permission checks are run, and + the `can_` fields return None. """ @property def _model(self) -> Type: @@ -235,7 +237,7 @@ def _model(self) -> Type: """ return Tag - def get_can_change(self, instance) -> bool: + def get_can_change(self, instance) -> Optional[bool]: """ Returns True if the current user is allowed to edit/change this Tag instance. @@ -243,9 +245,9 @@ def get_can_change(self, instance) -> bool: """ if isinstance(instance, Tag): return super()._can('change', instance) - return False + return None - def get_can_delete(self, instance) -> bool: + def get_can_delete(self, instance) -> Optional[bool]: """ Returns True if the current user is allowed to delete this Tag instance. @@ -253,7 +255,7 @@ def get_can_delete(self, instance) -> bool: """ if isinstance(instance, Tag): return super()._can('delete', instance) - return False + return None def to_representation(self, instance: TagData | Tag) -> dict: """ diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 7e21974f..a0241847 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -366,8 +366,6 @@ class ObjectTagView( **Retrieve Parameters** * object_id (required): - The Object ID to retrieve ObjectTags for. - - **Retrieve Query Parameters** * taxonomy (optional) - PK of taxonomy to filter ObjectTags for. **Retrieve Example Requests** @@ -379,6 +377,28 @@ class ObjectTagView( * 400 - Invalid query parameter * 403 - Permission denied + Response: + { + ${object_id}: { + taxonomies: [ + { + taxonomy_id: str, + can_tag_object: bool, + tags: [ + { + value: str, + lineage: list[str], + can_change: bool, // TODO: when/will free text tags are editable + can_delete: bool, + }, + ] + }, + ... + ], + }, + ... + } + **Update Parameters** * object_id (required): - The Object ID to add ObjectTags for. @@ -587,6 +607,7 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): Using full_depth_threshold=1000 is recommended in general, but use lower values during development to ensure compatibility with both large and small result sizes. * include_counts (optional) - Include the count of how many times each tag has been used. + * include_perms (optional) - Run permission checks (can_change, can_delete) and include in response. * page (optional) - Page number (default: 1) * page_size (optional) - Number of items per page (default: 30). Ignored when there are fewer tags than specified by ?full_depth_threshold. @@ -601,6 +622,24 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): * 403 - Permission denied * 404 - Taxonomy not found + Response: + + [ + { + value: str, + external_id: str, + child_count: int, + depth: int, + parent_value: str, + usage_count: int, + _id: int, + sub_tags_url: str, + can_change: bool|null, + can_delete: bool|null, + }, + ... + ], + **Create Query Parameters** * id (required) - The ID of the taxonomy to create a Tag for diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 04e3e09a..a31a0253 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -53,8 +53,8 @@ def check_taxonomy( allow_free_text=False, system_defined=False, visible_to_authors=True, - can_change=False, - can_delete=False, + can_change=None, + can_delete=None, ): """ Check taxonomy data @@ -145,7 +145,7 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c ) tag.save() - url = TAXONOMY_LIST_URL + url = TAXONOMY_LIST_URL + '?include_perms' if user_attr: user = getattr(self, user_attr) @@ -209,8 +209,6 @@ def test_list_taxonomy_pagination(self) -> None: next_page = parse_qs(parsed_url.query).get("page", [""])[0] assert next_page == "3" - assert response.data.get("can_add") - @ddt.data( ('user', False), ('staff', True), @@ -222,7 +220,7 @@ def test_list_taxonomy_empty(self, user_attr, expected_can_add) -> None: if language_taxonomy: language_taxonomy.delete() - url = TAXONOMY_LIST_URL + url = TAXONOMY_LIST_URL + '?include_perms' user = getattr(self, user_attr) self.client.force_authenticate(user=user) response = self.client.get(url, format="json") @@ -239,6 +237,33 @@ def test_list_taxonomy_empty(self, user_attr, expected_can_add) -> None: "can_add": expected_can_add, } + @ddt.data( + True, False + ) + def test_list_taxonomy_query_count(self, include_perms): + """ + Test how many queries are used when retrieving taxonomies and permissions + """ + api.create_taxonomy(name="T1", enabled=True) + api.create_taxonomy(name="T2", enabled=True) + + expected_perm = None + url = TAXONOMY_LIST_URL + if include_perms: + url += '?include_perms' + expected_perm = False + + self.client.force_authenticate(user=self.user) + with self.assertNumQueries(5): + response = self.client.get(url) + + assert response.status_code == 200 + assert response.data["can_add"] == expected_perm + assert len(response.data["results"]) == 3 + for taxonomy in response.data["results"]: + assert taxonomy["can_change"] == expected_perm + assert taxonomy["can_delete"] == expected_perm + def test_list_invalid_page(self) -> None: url = TAXONOMY_LIST_URL @@ -266,10 +291,7 @@ 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, - can_change=False, - can_delete=False, ) - assert not response.data.get("can_add") @ddt.data( (None, {"enabled": True}, status.HTTP_401_UNAUTHORIZED), @@ -285,7 +307,7 @@ def test_detail_taxonomy( ): 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) + url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) + '?include_perms' if user_attr: user = getattr(self, user_attr) @@ -346,14 +368,11 @@ 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): - expected_data = create_data - expected_data["can_change"] = True - expected_data["can_delete"] = True - check_taxonomy(response.data, response.data["id"], **expected_data) + check_taxonomy(response.data, response.data["id"], **create_data) url = TAXONOMY_DETAIL_URL.format(pk=response.data["id"]) response = self.client.get(url) - check_taxonomy(response.data, response.data["id"], **expected_data) + check_taxonomy(response.data, response.data["id"], **create_data) @ddt.data( {}, @@ -402,7 +421,7 @@ def test_update_taxonomy(self, user_attr, expected_status): # If we were able to update the taxonomy, check if the name changed if status.is_success(expected_status): - response = self.client.get(url) + response = self.client.get(url + '?include_perms') check_taxonomy( response.data, response.data["id"], @@ -461,7 +480,7 @@ def test_patch_taxonomy(self, user_attr, expected_status): # If we were able to update the taxonomy, check if the name changed if status.is_success(expected_status): - response = self.client.get(url) + response = self.client.get(url + '?include_perms') check_taxonomy( response.data, response.data["id"], @@ -677,7 +696,7 @@ def test_retrieve_object_tags(self, user_attr, expected_status): api.tag_object(object_id=object_id, taxonomy=self.taxonomy, tags=["Mammalia", "Fungi"]) api.tag_object(object_id=object_id, taxonomy=self.user_taxonomy, tags=[self.user_1.username]) - url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) + url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) + '?include_perms' if user_attr: user = getattr(self, user_attr) @@ -731,7 +750,7 @@ def test_retrieve_object_tags(self, user_attr, expected_status): }, } - def prepare_for_sort_test(self) -> tuple[str, list[dict]]: + def prepare_for_sort_test(self, expected_perm=None) -> tuple[str, list[dict]]: """ Tag an object with tags from the "sort test" taxonomy """ @@ -749,15 +768,16 @@ def prepare_for_sort_test(self) -> tuple[str, list[dict]]: "1111-grandchild", ] - shared_props = { - "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) + # Properties shared by all returned object tags + shared_props = { + "can_change": expected_perm, + "can_delete": expected_perm, + } + # The result we expect to see when retrieving the object tags, after applying the list above. # Note: the full taxonomy looks like the following, so this is the order we # expect, although not all of these tags were included. @@ -806,18 +826,26 @@ def test_retrieve_object_tags_sorted(self): assert response.data[object_id]["taxonomies"][0]["name"] == "Sort Test" assert response.data[object_id]["taxonomies"][0]["tags"] == sort_test_applied_result - def test_retrieve_object_tags_query_count(self): + @ddt.data( + True, False + ) + def test_retrieve_object_tags_query_count(self, include_perms): """ - Test how many queries are used when retrieving object tags + Test how many queries are used when retrieving object tags and permissions """ - object_id, sort_test_applied_result = self.prepare_for_sort_test() - + expected_perm = True if include_perms else None + object_id, sort_test_applied_result = self.prepare_for_sort_test(expected_perm) url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) + if include_perms: + url += '?include_perms' + self.client.force_authenticate(user=self.user_1) with self.assertNumQueries(1): response = self.client.get(url) assert response.status_code == 200 + assert len(response.data[object_id]["taxonomies"]) == 1 + assert response.data[object_id]["taxonomies"][0]["can_tag_object"] == expected_perm assert response.data[object_id]["taxonomies"][0]["tags"] == sort_test_applied_result def test_retrieve_object_tags_unauthorized(self): @@ -870,8 +898,8 @@ def test_retrieve_object_tags_taxonomy_queryparam( { "value": "test_user_1", "lineage": ["test_user_1"], - "can_change": True, - "can_delete": True, + "can_change": None, + "can_delete": None, }, ], } @@ -1161,6 +1189,7 @@ def test_get_counts_invalid_spec(self): assert "Wildcard matches are only supported if the * is at the end." in str(result.content) +@ddt.data class TestTaxonomyTagsView(TestTaxonomyViewMixin): """ Tests the list/create/update/delete tags of taxonomy view @@ -1229,7 +1258,7 @@ def test_small_taxonomy_root(self): Test explicitly requesting only the root tags of a small taxonomy. """ self.client.force_authenticate(user=self.staff) - response = self.client.get(self.small_taxonomy_url + "?include_counts") + response = self.client.get(self.small_taxonomy_url + "?include_counts&include_perms") assert response.status_code == status.HTTP_200_OK data = response.data @@ -1301,7 +1330,6 @@ def test_small_taxonomy(self): assert data.get("count") == len(results) assert data.get("num_pages") == 1 assert data.get("current_page") == 1 - assert data.get("can_add") def test_small_taxonomy_paged(self): """ @@ -1323,7 +1351,6 @@ def test_small_taxonomy_paged(self): assert data.get("count") == 3 assert data.get("num_pages") == 2 assert data.get("current_page") == 1 - assert data.get("can_add") # Get the next page: next_response = self.client.get(data.get("next")) @@ -1333,7 +1360,6 @@ def test_small_taxonomy_paged(self): "Eukaryota (None) (children: 5)", ] assert next_data.get("current_page") == 2 - assert next_data.get("can_add") def test_small_search(self): """ @@ -1360,7 +1386,6 @@ def test_small_search(self): assert data.get("count") == 5 assert data.get("num_pages") == 1 assert data.get("current_page") == 1 - assert data.get("can_add") def test_small_search_shallow(self): """ @@ -1385,7 +1410,6 @@ def test_small_search_shallow(self): assert data.get("count") == 3 assert data.get("num_pages") == 1 assert data.get("current_page") == 1 - assert data.get("can_add") # And we can load the sub_tags_url to "drill down" into the search: sub_tags_response = self.client.get(data["results"][0]["sub_tags_url"]) @@ -1396,6 +1420,30 @@ def test_small_search_shallow(self): " Euryarchaeida (children: 0)", ] + @ddt.data( + True, False + ) + def test_small_query_count(self, include_perms): + """ + Test how many queries are used when retrieving small taxonomies+tags and permissions + """ + expected_perm = None + url = f"{self.small_taxonomy_url}?search_term=eU" + if include_perms: + url += '?include_perms' + expected_perm = True + + self.client.force_authenticate(user=self.staff) + with self.assertNumQueries(1): + response = self.client.get(url) + + assert response.status_code == status.HTTP_200_OK + assert response.data["can_add"] == expected_perm + assert len(response.data["results"]) == 3 + for taxonomy in response.data["results"]: + assert taxonomy["can_change"] == expected_perm + assert taxonomy["can_delete"] == expected_perm + def test_empty_results(self): """ Test that various queries return an empty list @@ -1407,7 +1455,6 @@ def assert_empty(url): assert response.status_code == status.HTTP_200_OK assert response.data["results"] == [] assert response.data["count"] == 0 - assert response.data.get("can_add") # Search terms that won't match any tags: assert_empty(f"{self.small_taxonomy_url}?search_term=foobar") @@ -1419,13 +1466,25 @@ def assert_empty(url): assert_empty(f"{self.small_taxonomy_url}?search_term=eu&parent_tag=Euryarchaeida") assert_empty(f"{self.small_taxonomy_url}?search_term=eu&parent_tag=Euryarchaeida&full_depth_threshold=1000") - def test_large_taxonomy(self): + @ddt.data( + True, False, + ) + def test_large_taxonomy(self, include_perms): """ Test listing the tags in a large taxonomy (~7,000 tags). """ self._build_large_taxonomy() self.client.force_authenticate(user=self.staff) - response = self.client.get(self.large_taxonomy_url + "?include_counts") + + expected_perm = None + url = self.large_taxonomy_url + "?include_counts" + if include_perms: + url += "&include_perms" + expected_perm = True + + with self.assertNumQueries(1): + response = self.client.get(url) + assert response.status_code == status.HTTP_200_OK data = response.data @@ -1456,6 +1515,8 @@ def test_large_taxonomy(self): f"rest_api/v1/taxonomies/{self.large_taxonomy.id}" f"/tags/?parent_tag={quote_plus(results[0]['value'])}" ) + assert results[0].get("can_change") == expected_perm + assert results[0].get("can_delete") == expected_perm # Checking pagination values assert data.get("next") == ( @@ -1466,7 +1527,6 @@ def test_large_taxonomy(self): assert data.get("count") == self.root_tags_count assert data.get("num_pages") == 6 assert data.get("current_page") == 1 - assert data.get("can_add") def test_next_page_large_taxonomy(self): self._build_large_taxonomy() @@ -1493,7 +1553,6 @@ def test_next_page_large_taxonomy(self): assert data.get("count") == self.root_tags_count assert data.get("num_pages") == 6 assert data.get("current_page") == 2 - assert data.get("can_add") def test_large_search(self): """ @@ -1539,7 +1598,6 @@ def test_large_search(self): assert len(results) == expected_num_results assert data.get("num_pages") == 1 assert data.get("current_page") == 1 - assert data.get("can_add") # Now, perform the search with full_depth_threshold=100, which will give us paginated results, since there are # more than 100 matches: @@ -1564,7 +1622,6 @@ def test_large_search(self): assert data2.get("count") == 51 assert data2.get("num_pages") == 6 assert data2.get("current_page") == 1 - assert data2.get("can_add") # Now load the results that are in the subtree of the root tag 'Tag 0' tag_0_subtags_url = results2[0]["sub_tags_url"] @@ -1587,7 +1644,6 @@ def test_large_search(self): " Tag 118 (Tag 0) (children: 1)", " Tag 119 (Tag 118) (children: 0)", ] - assert data2.get("can_add") def test_get_children(self): self._build_large_taxonomy() @@ -1628,7 +1684,6 @@ def test_get_children(self): assert data.get("count") == self.children_tags_count[0] assert data.get("num_pages") == 2 assert data.get("current_page") == 1 - assert data.get("can_add") def test_get_leaves(self): """ @@ -1656,7 +1711,6 @@ def test_get_leaves(self): " Porifera (Animalia) (children: 0)", ] assert response.data.get("next") is None - assert response.data.get("can_add") def test_get_leaves_paginated(self): """ @@ -1682,7 +1736,6 @@ def test_get_leaves_paginated(self): ] next_url = response.data.get("next") assert next_url is not None - assert response.data.get("can_add") response2 = self.client.get(next_url) results2 = response2.data["results"] @@ -1690,7 +1743,6 @@ def test_get_leaves_paginated(self): " Placozoa (Animalia) (children: 0)", " Porifera (Animalia) (children: 0)", ] - assert response2.data.get("can_add") def test_next_children(self): self._build_large_taxonomy() @@ -1725,7 +1777,6 @@ def test_next_children(self): assert data.get("count") == self.children_tags_count[0] assert data.get("num_pages") == 2 assert data.get("current_page") == 2 - assert data.get("can_add") def test_create_tag_in_taxonomy_while_loggedout(self): new_tag_value = "New Tag" From 1e2d9b0066400aa4adf0e87ed9e18f25e212da2a Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 12 Jan 2024 02:06:06 +1030 Subject: [PATCH 10/17] feat: adds can_tag_object permission to Taxonomy List response This permission is needed by the Content Tag Drawer, which fetches the list of taxonomies available for tagging a given content item, and shows an "Add tags" button for the ones the user may add Object Tags for. --- .../core/tagging/rest_api/v1/serializers.py | 20 +++++++++++++++++++ .../core/tagging/test_views.py | 12 ++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index 22d2ec33..6b6a514e 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -13,6 +13,7 @@ from openedx_tagging.core.tagging.data import TagData from openedx_tagging.core.tagging.import_export.parsers import ParserFormat from openedx_tagging.core.tagging.models import ObjectTag, Tag, TagImportTask, Taxonomy +from openedx_tagging.core.tagging.rules import ObjectTagPermissionItem from ..utils import UserPermissionsHelper @@ -72,6 +73,7 @@ class TaxonomySerializer(UserPermissionsSerializerMixin, serializers.ModelSerial tags_count = serializers.SerializerMethodField() can_change = serializers.SerializerMethodField() can_delete = serializers.SerializerMethodField() + can_tag_object = serializers.SerializerMethodField() class Meta: model = Taxonomy @@ -87,6 +89,7 @@ class Meta: "tags_count", "can_change", "can_delete", + "can_tag_object", ] def to_representation(self, instance): @@ -99,6 +102,23 @@ def to_representation(self, instance): def get_tags_count(self, instance): return instance.tag_set.count() + def get_can_tag_object(self, instance) -> Optional[bool]: + """ + Returns True if the current request user may create object tags on this taxonomy. + + (The object_id test is necessarily skipped because we don't have an object_id to check.) + """ + request = self._request + assert request and request.user + if not self._include_perms: + return None + + model = self._model + app_label = model._meta.app_label + perm_name = f'{app_label}.add_objecttag' + perm_object = ObjectTagPermissionItem(taxonomy=instance, object_id=None) + return request.user.has_perm(perm_name, perm_object) + class ObjectTagListQueryParamsSerializer(serializers.Serializer): # pylint: disable=abstract-method """ diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index a31a0253..bb967272 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -55,6 +55,7 @@ def check_taxonomy( visible_to_authors=True, can_change=None, can_delete=None, + can_tag_object=None, ): """ Check taxonomy data @@ -69,6 +70,7 @@ def check_taxonomy( assert data["visible_to_authors"] == visible_to_authors assert data["can_change"] == can_change assert data["can_delete"] == can_delete + assert data["can_tag_object"] == can_tag_object class TestTaxonomyViewMixin(APITestCase): @@ -170,6 +172,7 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c # System taxonomy cannot be modified "can_change": False, "can_delete": False, + "can_tag_object": False, }, { "id": taxonomy.id, @@ -184,6 +187,7 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c # Enabled taxonomy can be modified by staff "can_change": is_admin, "can_delete": is_admin, + "can_tag_object": False, }, ] assert response.data.get("can_add") == is_admin @@ -263,6 +267,7 @@ def test_list_taxonomy_query_count(self, include_perms): for taxonomy in response.data["results"]: assert taxonomy["can_change"] == expected_perm assert taxonomy["can_delete"] == expected_perm + assert taxonomy["can_tag_object"] == expected_perm def test_list_invalid_page(self) -> None: url = TAXONOMY_LIST_URL @@ -320,6 +325,7 @@ def test_detail_taxonomy( expected_data = create_data expected_data["can_change"] = is_admin expected_data["can_delete"] = is_admin + expected_data["can_tag_object"] = False check_taxonomy(response.data, taxonomy.pk, **expected_data) def test_detail_system_taxonomy(self): @@ -431,6 +437,7 @@ def test_update_taxonomy(self, user_attr, expected_status): "enabled": True, "can_change": True, "can_delete": True, + "can_tag_object": False, }, ) @@ -489,6 +496,7 @@ def test_patch_taxonomy(self, user_attr, expected_status): "enabled": True, "can_change": True, "can_delete": True, + "can_tag_object": False, }, ) @@ -1430,7 +1438,7 @@ def test_small_query_count(self, include_perms): expected_perm = None url = f"{self.small_taxonomy_url}?search_term=eU" if include_perms: - url += '?include_perms' + url += '&include_perms' expected_perm = True self.client.force_authenticate(user=self.staff) @@ -1443,6 +1451,7 @@ def test_small_query_count(self, include_perms): for taxonomy in response.data["results"]: assert taxonomy["can_change"] == expected_perm assert taxonomy["can_delete"] == expected_perm + assert not taxonomy["can_tag_object"] def test_empty_results(self): """ @@ -1517,6 +1526,7 @@ def test_large_taxonomy(self, include_perms): ) assert results[0].get("can_change") == expected_perm assert results[0].get("can_delete") == expected_perm + assert not results[0].get("can_tag_object") # Checking pagination values assert data.get("next") == ( From bb1a0a209889343e1d7a23d6028b61cf582b425f Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 12 Jan 2024 06:00:22 +1030 Subject: [PATCH 11/17] fix: feat: check permissions only if ?include_perms fixes type/testing issues with previous commit --- .../core/tagging/rest_api/v1/serializers.py | 4 ++-- .../core/tagging/test_views.py | 24 ++++++++++--------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index 6b6a514e..633868f4 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -116,8 +116,8 @@ def get_can_tag_object(self, instance) -> Optional[bool]: model = self._model app_label = model._meta.app_label perm_name = f'{app_label}.add_objecttag' - perm_object = ObjectTagPermissionItem(taxonomy=instance, object_id=None) - return request.user.has_perm(perm_name, perm_object) + perm_object = ObjectTagPermissionItem(taxonomy=instance, object_id="") + return request.user.has_perm(perm_name, perm_object) # type: ignore[arg-type] class ObjectTagListQueryParamsSerializer(serializers.Serializer): # pylint: disable=abstract-method diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index bb967272..c605ae1e 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -1197,7 +1197,7 @@ def test_get_counts_invalid_spec(self): assert "Wildcard matches are only supported if the * is at the end." in str(result.content) -@ddt.data +@ddt.ddt class TestTaxonomyTagsView(TestTaxonomyViewMixin): """ Tests the list/create/update/delete tags of taxonomy view @@ -1442,16 +1442,16 @@ def test_small_query_count(self, include_perms): expected_perm = True self.client.force_authenticate(user=self.staff) - with self.assertNumQueries(1): + with self.assertNumQueries(6): response = self.client.get(url) assert response.status_code == status.HTTP_200_OK assert response.data["can_add"] == expected_perm assert len(response.data["results"]) == 3 for taxonomy in response.data["results"]: - assert taxonomy["can_change"] == expected_perm - assert taxonomy["can_delete"] == expected_perm - assert not taxonomy["can_tag_object"] + # TODO Permission checks are not run for TagData, only Tag instances. + assert taxonomy["can_change"] is None + assert taxonomy["can_delete"] is None def test_empty_results(self): """ @@ -1486,12 +1486,14 @@ def test_large_taxonomy(self, include_perms): self.client.force_authenticate(user=self.staff) expected_perm = None + expected_next_params = 'include_counts=&page=2' url = self.large_taxonomy_url + "?include_counts" if include_perms: url += "&include_perms" expected_perm = True + expected_next_params = 'include_counts=&include_perms=&page=2' - with self.assertNumQueries(1): + with self.assertNumQueries(4): response = self.client.get(url) assert response.status_code == status.HTTP_200_OK @@ -1524,19 +1526,20 @@ def test_large_taxonomy(self, include_perms): f"rest_api/v1/taxonomies/{self.large_taxonomy.id}" f"/tags/?parent_tag={quote_plus(results[0]['value'])}" ) - assert results[0].get("can_change") == expected_perm - assert results[0].get("can_delete") == expected_perm - assert not results[0].get("can_tag_object") + # TODO Permission checks are not run for TagData, only Tag instances. + assert results[0].get("can_change") is None + assert results[0].get("can_delete") is None # Checking pagination values assert data.get("next") == ( "http://testserver/tagging/" - f"rest_api/v1/taxonomies/{self.large_taxonomy.id}/tags/?include_counts=&page=2" + f"rest_api/v1/taxonomies/{self.large_taxonomy.id}/tags/?{expected_next_params}" ) assert data.get("previous") is None assert data.get("count") == self.root_tags_count assert data.get("num_pages") == 6 assert data.get("current_page") == 1 + assert data.get("can_add") == expected_perm def test_next_page_large_taxonomy(self): self._build_large_taxonomy() @@ -1746,7 +1749,6 @@ def test_get_leaves_paginated(self): ] next_url = response.data.get("next") assert next_url is not None - response2 = self.client.get(next_url) results2 = response2.data["results"] assert pretty_format_tags(results2) == [ From a6523131e2519e8ea46b67c13c25ef2fb1efa14c Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 12 Jan 2024 06:47:51 +1030 Subject: [PATCH 12/17] chore: bumps version --- openedx_learning/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index e77368e2..aeffa12d 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.4.2" +__version__ = "0.4.3" From f90b30508fa97f7b39512a51d930519ceef5e3c7 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 12 Jan 2024 11:00:25 +1030 Subject: [PATCH 13/17] fix: renames the `can_` fields to `can__` to alleviate confusion about what permissions they reflect. --- .../core/tagging/rest_api/paginators.py | 7 +- .../core/tagging/rest_api/utils.py | 27 ++++--- .../core/tagging/rest_api/v1/serializers.py | 46 +++++------ .../core/tagging/rest_api/v1/views.py | 66 +++++++++------ .../core/tagging/test_views.py | 80 +++++++++---------- 5 files changed, 122 insertions(+), 104 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/paginators.py b/openedx_tagging/core/tagging/rest_api/paginators.py index f5d37816..5831e8b0 100644 --- a/openedx_tagging/core/tagging/rest_api/paginators.py +++ b/openedx_tagging/core/tagging/rest_api/paginators.py @@ -17,12 +17,10 @@ class CanAddPermissionMixin(UserPermissionsHelper): # pylint: disable=abstract-method """ - This mixin inserts a boolean "can_add" field at the top level of the paginated response. + This mixin inserts a boolean "can_add_" field at the top level of the paginated response. The value of the field indicates whether request user may create new instances of the current model. """ - field_name = 'can_add' - @property def _request(self) -> Request: """ @@ -35,7 +33,8 @@ def get_paginated_response(self, data) -> Response: Injects the user's model-level permissions into the paginated response. """ response_data = super().get_paginated_response(data).data # type: ignore[misc] - response_data[self.field_name] = self.get_can_add() + field_name = f"can_add_{self.model_name}" + response_data[field_name] = self.get_can_add() return Response(response_data) diff --git a/openedx_tagging/core/tagging/rest_api/utils.py b/openedx_tagging/core/tagging/rest_api/utils.py index 9e7fa0a7..0784eb90 100644 --- a/openedx_tagging/core/tagging/rest_api/utils.py +++ b/openedx_tagging/core/tagging/rest_api/utils.py @@ -31,6 +31,13 @@ class UserPermissionsHelper: """ Provides helper methods for serializing user permissions. """ + @property + def _request(self) -> Request: + """ + Returns the current request. + """ + raise NotImplementedError # pragma: no cover + @property def _model(self) -> Type: """ @@ -39,11 +46,18 @@ def _model(self) -> Type: raise NotImplementedError # pragma: no cover @property - def _request(self) -> Request: + def app_label(self) -> Type: """ - Returns the current request. + Returns the app_label for the model used when checking permissions. """ - raise NotImplementedError # pragma: no cover + return self._model._meta.app_label + + @property + def model_name(self) -> Type: + """ + Returns the name of the model used when checking permissions. + """ + return self._model._meta.model_name @property def _include_perms(self) -> bool: @@ -70,12 +84,7 @@ def _can(self, action: str, instance=None) -> Optional[bool]: assert action in ("add", "view", "change", "delete") - model = self._model - assert model - - app_label = model._meta.app_label - model_name = model._meta.model_name - perm_name = f'{app_label}.{action}_{model_name}' + perm_name = f'{self.app_label}.{action}_{self.model_name}' return request.user.has_perm(perm_name, instance) def get_can_add(self, _instance=None) -> Optional[bool]: diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index 633868f4..cdb009a8 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -71,8 +71,8 @@ class TaxonomySerializer(UserPermissionsSerializerMixin, serializers.ModelSerial Serializer for the Taxonomy model. """ tags_count = serializers.SerializerMethodField() - can_change = serializers.SerializerMethodField() - can_delete = serializers.SerializerMethodField() + can_change_taxonomy = serializers.SerializerMethodField(method_name='get_can_change') + can_delete_taxonomy = serializers.SerializerMethodField(method_name='get_can_delete') can_tag_object = serializers.SerializerMethodField() class Meta: @@ -87,8 +87,8 @@ class Meta: "system_defined", "visible_to_authors", "tags_count", - "can_change", - "can_delete", + "can_change_taxonomy", + "can_delete_taxonomy", "can_tag_object", ] @@ -108,16 +108,11 @@ def get_can_tag_object(self, instance) -> Optional[bool]: (The object_id test is necessarily skipped because we don't have an object_id to check.) """ - request = self._request - assert request and request.user - if not self._include_perms: + if not (self._request and self._include_perms): return None - - model = self._model - app_label = model._meta.app_label - perm_name = f'{app_label}.add_objecttag' + perm_name = f'{self.app_label}.add_objecttag' perm_object = ObjectTagPermissionItem(taxonomy=instance, object_id="") - return request.user.has_perm(perm_name, perm_object) # type: ignore[arg-type] + return self._request.user.has_perm(perm_name, perm_object) # type: ignore[arg-type] class ObjectTagListQueryParamsSerializer(serializers.Serializer): # pylint: disable=abstract-method @@ -137,11 +132,11 @@ class ObjectTagMinimalSerializer(UserPermissionsSerializerMixin, serializers.Mod class Meta: model = ObjectTag - fields = ["value", "lineage", "can_change", "can_delete"] + fields = ["value", "lineage", "can_change_objecttag", "can_delete_objecttag"] lineage = serializers.ListField(child=serializers.CharField(), source="get_lineage", read_only=True) - can_change = serializers.SerializerMethodField() - can_delete = serializers.SerializerMethodField() + can_change_objecttag = serializers.SerializerMethodField(method_name='get_can_change') + can_delete_objecttag = serializers.SerializerMethodField(method_name='get_can_delete') class ObjectTagSerializer(ObjectTagMinimalSerializer): @@ -225,8 +220,8 @@ class TagDataSerializer(UserPermissionsSerializerMixin, serializers.Serializer): _id = serializers.IntegerField(allow_null=True) sub_tags_url = serializers.SerializerMethodField() - can_change = serializers.SerializerMethodField() - can_delete = serializers.SerializerMethodField() + can_change_tag = serializers.SerializerMethodField() + can_delete_tag = serializers.SerializerMethodField() def get_sub_tags_url(self, obj: TagData | Tag): """ @@ -257,25 +252,22 @@ def _model(self) -> Type: """ return Tag - def get_can_change(self, instance) -> Optional[bool]: + def get_can_change_tag(self, _instance) -> Optional[bool]: """ Returns True if the current user is allowed to edit/change this Tag instance. - Returns False for all TagData instances. + Because we're serializing TagData (not Tags), the view stores these permissions in the serializer + context. """ - if isinstance(instance, Tag): - return super()._can('change', instance) - return None + return self.context.get('can_change_tag') - def get_can_delete(self, instance) -> Optional[bool]: + def get_can_delete_tag(self, _instance) -> Optional[bool]: """ Returns True if the current user is allowed to delete this Tag instance. - Returns False for all TagData instances. + Because we're serializing TagData (not Tags), the view stores these permissions in the serializer """ - if isinstance(instance, Tag): - return super()._can('delete', instance) - return None + return self.context.get('can_delete_tag') def to_representation(self, instance: TagData | Tag) -> dict: """ diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index a0241847..be3b2f53 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -28,7 +28,7 @@ from ...data import TagDataQuerySet from ...import_export.api import export_tags, import_tags from ...import_export.parsers import ParserFormat -from ...models import Taxonomy +from ...models import Tag, Taxonomy from ...rules import ObjectTagPermissionItem from ..paginators import MAX_FULL_DEPTH_THRESHOLD, DisabledTagsPagination, TagsPagination, TaxonomyPagination from ..utils import view_auth_classes @@ -388,8 +388,7 @@ class ObjectTagView( { value: str, lineage: list[str], - can_change: bool, // TODO: when/will free text tags are editable - can_delete: bool, + can_delete_objecttag: bool, }, ] }, @@ -607,7 +606,8 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): Using full_depth_threshold=1000 is recommended in general, but use lower values during development to ensure compatibility with both large and small result sizes. * include_counts (optional) - Include the count of how many times each tag has been used. - * include_perms (optional) - Run permission checks (can_change, can_delete) and include in response. + * include_perms (optional) - Include permission checks in response. + If omitted, permission fields (can_add_tag, can_change_tag, can_delete_tag) will be None. * page (optional) - Page number (default: 1) * page_size (optional) - Number of items per page (default: 30). Ignored when there are fewer tags than specified by ?full_depth_threshold. @@ -624,7 +624,8 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): Response: - [ + can_add_tag: bool, + results: [ { value: str, external_id: str, @@ -634,8 +635,7 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): usage_count: int, _id: int, sub_tags_url: str, - can_change: bool|null, - can_delete: bool|null, + can_delete_tag: bool|null, }, ... ], @@ -713,21 +713,45 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): pagination_class = TagsPagination serializer_class = TagDataSerializer - def get_taxonomy(self, pk: int) -> Taxonomy: + def __init__(self, *args, **kwargs): + """ + Initializes the local variables. + """ + super().__init__(*args, **kwargs) + self._taxonomy = None + + def get_taxonomy(self) -> Taxonomy: """ Get the taxonomy from `pk` or raise 404. + + The current taxonomy is cached in the view. """ - taxonomy = get_taxonomy(pk) - if not taxonomy: - raise Http404("Taxonomy not found") - self.check_object_permissions(self.request, taxonomy) - return taxonomy + if not self._taxonomy: + taxonomy_id = int(self.kwargs["pk"]) + taxonomy = get_taxonomy(taxonomy_id) + if not taxonomy: + raise Http404("Taxonomy not found") + self.check_object_permissions(self.request, taxonomy) + self._taxonomy = taxonomy + return self._taxonomy def get_serializer_context(self): + """ + Passes data from the view to the serializer. + """ + # Use our serializer to check permissions if requested -- requires a request in the context. context = super().get_serializer_context() + context['request'] = self.request + serializer = self.serializer_class(self, context=context) + + # Instead of checking permissions for each TagData instance, we just check them once for the whole taxonomy + # (since that's currently how our rules work). This might change if Tag-specific permissions are needed. + taxonomy = self.get_taxonomy() + dummy_tag = Tag(taxonomy=taxonomy) context.update({ - "request": self.request, - "taxonomy_id": int(self.kwargs["pk"]), + "taxonomy_id": taxonomy.id, + "can_change_tag": serializer.get_can_change(dummy_tag), + "can_delete_tag": serializer.get_can_delete(dummy_tag), }) return context @@ -735,8 +759,7 @@ def get_queryset(self) -> TagDataQuerySet: """ Builds and returns the queryset to be paginated. """ - taxonomy_id = int(self.kwargs.get("pk")) - taxonomy = self.get_taxonomy(taxonomy_id) + taxonomy = self.get_taxonomy() parent_tag_value = self.request.query_params.get("parent_tag", None) include_counts = "include_counts" in self.request.query_params search_term = self.request.query_params.get("search_term", None) @@ -779,8 +802,7 @@ def post(self, request, *args, **kwargs): """ Creates new Tag in Taxonomy and returns the newly created Tag. """ - pk = self.kwargs.get("pk") - taxonomy = self.get_taxonomy(pk) + taxonomy = self.get_taxonomy() body = TaxonomyTagCreateBodySerializer(data=request.data) body.is_valid(raise_exception=True) @@ -809,8 +831,7 @@ def update(self, request, *args, **kwargs): Updates a Tag that belongs to the Taxonomy and returns it. Currently only updating the Tag value is supported. """ - pk = self.kwargs.get("pk") - taxonomy = self.get_taxonomy(pk) + taxonomy = self.get_taxonomy() body = TaxonomyTagUpdateBodySerializer(data=request.data) body.is_valid(raise_exception=True) @@ -837,8 +858,7 @@ def delete(self, request, *args, **kwargs): the `with_subtags` is not set to `True` it will fail, otherwise the sub-tags will be deleted as well. """ - pk = self.kwargs.get("pk") - taxonomy = self.get_taxonomy(pk) + taxonomy = self.get_taxonomy() body = TaxonomyTagDeleteBodySerializer(data=request.data) body.is_valid(raise_exception=True) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index c605ae1e..f296fb0b 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -53,8 +53,8 @@ def check_taxonomy( allow_free_text=False, system_defined=False, visible_to_authors=True, - can_change=None, - can_delete=None, + can_change_taxonomy=None, + can_delete_taxonomy=None, can_tag_object=None, ): """ @@ -68,8 +68,8 @@ 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["can_change"] == can_change - assert data["can_delete"] == can_delete + assert data["can_change_taxonomy"] == can_change_taxonomy + assert data["can_delete_taxonomy"] == can_delete_taxonomy assert data["can_tag_object"] == can_tag_object @@ -170,8 +170,8 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c "visible_to_authors": True, "tags_count": 0, # System taxonomy cannot be modified - "can_change": False, - "can_delete": False, + "can_change_taxonomy": False, + "can_delete_taxonomy": False, "can_tag_object": False, }, { @@ -184,13 +184,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 - "can_change": is_admin, - "can_delete": is_admin, + # Enabled taxonomy can be modified by taxonomy admins + "can_change_taxonomy": is_admin, + "can_delete_taxonomy": is_admin, "can_tag_object": False, }, ] - assert response.data.get("can_add") == is_admin + assert response.data.get("can_add_taxonomy") == is_admin def test_list_taxonomy_pagination(self) -> None: url = TAXONOMY_LIST_URL @@ -238,7 +238,7 @@ def test_list_taxonomy_empty(self, user_attr, expected_can_add) -> None: "previous": None, "start": 0, "results": [], - "can_add": expected_can_add, + "can_add_taxonomy": expected_can_add, } @ddt.data( @@ -262,11 +262,11 @@ def test_list_taxonomy_query_count(self, include_perms): response = self.client.get(url) assert response.status_code == 200 - assert response.data["can_add"] == expected_perm + assert response.data["can_add_taxonomy"] == expected_perm assert len(response.data["results"]) == 3 for taxonomy in response.data["results"]: - assert taxonomy["can_change"] == expected_perm - assert taxonomy["can_delete"] == expected_perm + assert taxonomy["can_change_taxonomy"] == expected_perm + assert taxonomy["can_delete_taxonomy"] == expected_perm assert taxonomy["can_tag_object"] == expected_perm def test_list_invalid_page(self) -> None: @@ -323,8 +323,8 @@ def test_detail_taxonomy( if status.is_success(expected_status): expected_data = create_data - expected_data["can_change"] = is_admin - expected_data["can_delete"] = is_admin + expected_data["can_change_taxonomy"] = is_admin + expected_data["can_delete_taxonomy"] = is_admin expected_data["can_tag_object"] = False check_taxonomy(response.data, taxonomy.pk, **expected_data) @@ -435,8 +435,8 @@ def test_update_taxonomy(self, user_attr, expected_status): "name": "new name", "description": "taxonomy description", "enabled": True, - "can_change": True, - "can_delete": True, + "can_change_taxonomy": True, + "can_delete_taxonomy": True, "can_tag_object": False, }, ) @@ -494,8 +494,8 @@ def test_patch_taxonomy(self, user_attr, expected_status): **{ "name": "new name", "enabled": True, - "can_change": True, - "can_delete": True, + "can_change_taxonomy": True, + "can_delete_taxonomy": True, "can_tag_object": False, }, ) @@ -730,14 +730,14 @@ def test_retrieve_object_tags(self, user_attr, expected_status): { "value": "Mammalia", "lineage": ["Eukaryota", "Animalia", "Chordata", "Mammalia"], - "can_change": True, - "can_delete": True, + "can_change_objecttag": True, + "can_delete_objecttag": True, }, { "value": "Fungi", "lineage": ["Eukaryota", "Fungi"], - "can_change": True, - "can_delete": True, + "can_change_objecttag": True, + "can_delete_objecttag": True, }, ] }, @@ -749,8 +749,8 @@ def test_retrieve_object_tags(self, user_attr, expected_status): { "value": "test_user_1", "lineage": ["test_user_1"], - "can_change": True, - "can_delete": True, + "can_change_objecttag": True, + "can_delete_objecttag": True, }, ], } @@ -782,8 +782,8 @@ def prepare_for_sort_test(self, expected_perm=None) -> tuple[str, list[dict]]: # Properties shared by all returned object tags shared_props = { - "can_change": expected_perm, - "can_delete": expected_perm, + "can_change_objecttag": expected_perm, + "can_delete_objecttag": expected_perm, } # The result we expect to see when retrieving the object tags, after applying the list above. @@ -906,8 +906,8 @@ def test_retrieve_object_tags_taxonomy_queryparam( { "value": "test_user_1", "lineage": ["test_user_1"], - "can_change": None, - "can_delete": None, + "can_change_objecttag": None, + "can_delete_objecttag": None, }, ], } @@ -1297,7 +1297,7 @@ def test_small_taxonomy_root(self): assert data.get("count") == root_count assert data.get("num_pages") == 1 assert data.get("current_page") == 1 - assert data.get("can_add") + assert data.get("can_add_tag") def test_small_taxonomy(self): """ @@ -1442,16 +1442,15 @@ def test_small_query_count(self, include_perms): expected_perm = True self.client.force_authenticate(user=self.staff) - with self.assertNumQueries(6): + with self.assertNumQueries(5): response = self.client.get(url) assert response.status_code == status.HTTP_200_OK - assert response.data["can_add"] == expected_perm + assert response.data["can_add_tag"] == expected_perm assert len(response.data["results"]) == 3 for taxonomy in response.data["results"]: - # TODO Permission checks are not run for TagData, only Tag instances. - assert taxonomy["can_change"] is None - assert taxonomy["can_delete"] is None + assert taxonomy["can_change_tag"] == expected_perm + assert taxonomy["can_delete_tag"] == expected_perm def test_empty_results(self): """ @@ -1493,7 +1492,7 @@ def test_large_taxonomy(self, include_perms): expected_perm = True expected_next_params = 'include_counts=&include_perms=&page=2' - with self.assertNumQueries(4): + with self.assertNumQueries(3): response = self.client.get(url) assert response.status_code == status.HTTP_200_OK @@ -1526,9 +1525,8 @@ def test_large_taxonomy(self, include_perms): f"rest_api/v1/taxonomies/{self.large_taxonomy.id}" f"/tags/?parent_tag={quote_plus(results[0]['value'])}" ) - # TODO Permission checks are not run for TagData, only Tag instances. - assert results[0].get("can_change") is None - assert results[0].get("can_delete") is None + assert results[0].get("can_change_tag") == expected_perm + assert results[0].get("can_delete_tag") == expected_perm # Checking pagination values assert data.get("next") == ( @@ -1539,7 +1537,7 @@ def test_large_taxonomy(self, include_perms): assert data.get("count") == self.root_tags_count assert data.get("num_pages") == 6 assert data.get("current_page") == 1 - assert data.get("can_add") == expected_perm + assert data.get("can_add_tag") == expected_perm def test_next_page_large_taxonomy(self): self._build_large_taxonomy() From 3e24b66222c457214e4a8b408678261b037a50ea Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 12 Jan 2024 18:27:47 +1030 Subject: [PATCH 14/17] fix: check add + delete permissions when updating ObjectTags for a given taxonomy + object_id, because we are adding and deleting ObjectTags here, not editing the individual instances. --- .../core/tagging/rest_api/v1/views.py | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index be3b2f53..e0ff5a0d 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -508,22 +508,24 @@ def update(self, request, *args, **kwargs) -> Response: taxonomy = query_params.validated_data.get("taxonomy", None) taxonomy = taxonomy.cast() - perm = "oel_tagging.change_objecttag" - object_id = kwargs.pop('object_id') perm_obj = ObjectTagPermissionItem( taxonomy=taxonomy, object_id=object_id, ) - - if not request.user.has_perm( - perm, - # The obj arg expects a model, but we are passing an object - perm_obj, # type: ignore[arg-type] - ): - raise PermissionDenied( - "You do not have permission to change object tags for this taxonomy or object_id." - ) + perms = [ + "oel_tagging.add_objecttag", + "oel_tagging.delete_objecttag" + ] + for perm in perms: + if not request.user.has_perm( + perm, + # The obj arg expects a model, but we are passing an object + perm_obj, # type: ignore[arg-type] + ): + raise PermissionDenied( + "You do not have permission to change object tags for this taxonomy or object_id." + ) body = ObjectTagUpdateBodySerializer(data=request.data) body.is_valid(raise_exception=True) From 3dfea4022501ec243b864efc6f47899e6d1f38da Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 18 Jan 2024 12:19:52 +1030 Subject: [PATCH 15/17] feat: adds "can_tag_object" rule which means a user may add or delete ObjectTags using a given Taxonomy + object_id. This permission is used in the bulk tag update REST API endpoint, and when serializing Taxonomies in the REST API. --- .../core/tagging/rest_api/utils.py | 27 ++++++++++++------- .../core/tagging/rest_api/v1/serializers.py | 9 +++---- .../core/tagging/rest_api/v1/views.py | 24 ++++++++--------- openedx_tagging/core/tagging/rules.py | 2 ++ 4 files changed, 34 insertions(+), 28 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/utils.py b/openedx_tagging/core/tagging/rest_api/utils.py index 0784eb90..ce44780f 100644 --- a/openedx_tagging/core/tagging/rest_api/utils.py +++ b/openedx_tagging/core/tagging/rest_api/utils.py @@ -69,9 +69,16 @@ def _include_perms(self) -> bool: """ return 'include_perms' in self._request.query_params - def _can(self, action: str, instance=None) -> Optional[bool]: + def _get_permission_name(self, action: str) -> str: """ - Can the current `request.user` perform the given `action` on the `instance` object? + Returns the fully-qualified permission name corresponding to the current model and `action`. + """ + assert action in ("add", "view", "change", "delete") + return f'{self.app_label}.{action}_{self.model_name}' + + def _can(self, perm_name: str, instance=None) -> Optional[bool]: + """ + Does the current `request.user` have the given `perm` on the `instance` object? Returns None if no permissions were requested. Returns True if they may. @@ -81,10 +88,6 @@ def _can(self, action: str, instance=None) -> Optional[bool]: assert request and request.user if not self._include_perms: return None - - assert action in ("add", "view", "change", "delete") - - perm_name = f'{self.app_label}.{action}_{self.model_name}' return request.user.has_perm(perm_name, instance) def get_can_add(self, _instance=None) -> Optional[bool]: @@ -93,22 +96,26 @@ def get_can_add(self, _instance=None) -> Optional[bool]: Note: we omit the actual instance from the permissions check; most tagging models prefer this. """ - return self._can('add') + perm_name = self._get_permission_name('add') + return self._can(perm_name) def get_can_view(self, instance) -> Optional[bool]: """ Returns True if the current user is allowed to view/see this instance. """ - return self._can('view', instance) + perm_name = self._get_permission_name('view') + return self._can(perm_name, instance) def get_can_change(self, instance) -> Optional[bool]: """ Returns True if the current user is allowed to edit/change this instance. """ - return self._can('change', instance) + perm_name = self._get_permission_name('change') + return self._can(perm_name, instance) def get_can_delete(self, instance) -> Optional[bool]: """ Returns True if the current user is allowed to delete this instance. """ - return self._can('delete', instance) + perm_name = self._get_permission_name('change') + return self._can(perm_name, instance) diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index cdb009a8..facc7fac 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -108,11 +108,9 @@ def get_can_tag_object(self, instance) -> Optional[bool]: (The object_id test is necessarily skipped because we don't have an object_id to check.) """ - if not (self._request and self._include_perms): - return None - perm_name = f'{self.app_label}.add_objecttag' + perm_name = f'{self.app_label}.can_tag_object' perm_object = ObjectTagPermissionItem(taxonomy=instance, object_id="") - return self._request.user.has_perm(perm_name, perm_object) # type: ignore[arg-type] + return self._can(perm_name, perm_object) class ObjectTagListQueryParamsSerializer(serializers.Serializer): # pylint: disable=abstract-method @@ -165,6 +163,7 @@ def to_representation(self, instance: list[ObjectTag]) -> dict: """ Convert this list of ObjectTags to the serialized dictionary, grouped by Taxonomy """ + can_tag_object_perm = f"{self.app_label}.can_tag_object" by_object: dict[str, dict[str, Any]] = {} for obj_tag in instance: if obj_tag.object_id not in by_object: @@ -177,7 +176,7 @@ def to_representation(self, instance: list[ObjectTag]) -> dict: tax_entry = { "name": obj_tag.name, "taxonomy_id": obj_tag.taxonomy_id, - "can_tag_object": self._can('add', obj_tag), + "can_tag_object": self._can(can_tag_object_perm, obj_tag), "tags": [] } taxonomies.append(tax_entry) diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index e0ff5a0d..85178fe9 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -508,24 +508,22 @@ def update(self, request, *args, **kwargs) -> Response: taxonomy = query_params.validated_data.get("taxonomy", None) taxonomy = taxonomy.cast() + perm = "oel_tagging.can_tag_object" + object_id = kwargs.pop('object_id') perm_obj = ObjectTagPermissionItem( taxonomy=taxonomy, object_id=object_id, ) - perms = [ - "oel_tagging.add_objecttag", - "oel_tagging.delete_objecttag" - ] - for perm in perms: - if not request.user.has_perm( - perm, - # The obj arg expects a model, but we are passing an object - perm_obj, # type: ignore[arg-type] - ): - raise PermissionDenied( - "You do not have permission to change object tags for this taxonomy or object_id." - ) + + if not request.user.has_perm( + perm, + # The obj arg expects a model, but we are passing an object + perm_obj, # type: ignore[arg-type] + ): + raise PermissionDenied( + "You do not have permission to change object tags for this taxonomy or object_id." + ) body = ObjectTagUpdateBodySerializer(data=request.data) body.is_valid(raise_exception=True) diff --git a/openedx_tagging/core/tagging/rules.py b/openedx_tagging/core/tagging/rules.py index 76139244..36f67ffe 100644 --- a/openedx_tagging/core/tagging/rules.py +++ b/openedx_tagging/core/tagging/rules.py @@ -186,6 +186,8 @@ def can_change_object_tag( rules.add_perm("oel_tagging.change_objecttag", can_change_object_tag) rules.add_perm("oel_tagging.delete_objecttag", can_change_object_tag) rules.add_perm("oel_tagging.view_objecttag", can_view_object_tag) +# If a user "can tag object", they can delete or create ObjectTags using the given Taxonomy + object_id. +rules.add_perm("oel_tagging.can_tag_object", can_change_object_tag) # Users can tag objects using tags from any taxonomy that they have permission to view rules.add_perm("oel_tagging.view_objecttag_objectid", can_view_object_tag_objectid) From 786ef8b137ddc93d5b6d6871316f70b56343db5d Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 23 Jan 2024 12:16:38 +1030 Subject: [PATCH 16/17] revert: removes the include_perms querystring param from the tagging views We've demonstrated that adding permissions to the REST API does not add more database queries, so we can always return permissions. --- openedx_learning/__init__.py | 2 +- .../core/tagging/rest_api/utils.py | 12 --- .../core/tagging/rest_api/v1/serializers.py | 2 - .../core/tagging/rest_api/v1/views.py | 2 - .../core/tagging/test_views.py | 92 +++++++------------ 5 files changed, 35 insertions(+), 75 deletions(-) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index aeffa12d..13243d24 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.4.3" +__version__ = "0.4.4" diff --git a/openedx_tagging/core/tagging/rest_api/utils.py b/openedx_tagging/core/tagging/rest_api/utils.py index ce44780f..ed68de50 100644 --- a/openedx_tagging/core/tagging/rest_api/utils.py +++ b/openedx_tagging/core/tagging/rest_api/utils.py @@ -59,16 +59,6 @@ def model_name(self) -> Type: """ return self._model._meta.model_name - @property - def _include_perms(self) -> bool: - """ - Are permission checks requested? - - Returns True if ?include_perms found in the query string - Returns False by default. - """ - return 'include_perms' in self._request.query_params - def _get_permission_name(self, action: str) -> str: """ Returns the fully-qualified permission name corresponding to the current model and `action`. @@ -86,8 +76,6 @@ def _can(self, perm_name: str, instance=None) -> Optional[bool]: """ request = self._request assert request and request.user - if not self._include_perms: - return None return request.user.has_perm(perm_name, instance) def get_can_add(self, _instance=None) -> Optional[bool]: diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index facc7fac..15dc31fe 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -48,8 +48,6 @@ class UserPermissionsSerializerMixin(UserPermissionsHelper): Notes: * Assumes the serialized model should be used to check permissions (override _model to change). * Requires the current request to be passed into the serializer context (override _request to change). - * Requires '?include_perms` in the request, otherwise no permission checks are run, and - the `can_` fields return None. """ @property def _model(self) -> Type: diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index 85178fe9..3abf4f0d 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -606,8 +606,6 @@ class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): Using full_depth_threshold=1000 is recommended in general, but use lower values during development to ensure compatibility with both large and small result sizes. * include_counts (optional) - Include the count of how many times each tag has been used. - * include_perms (optional) - Include permission checks in response. - If omitted, permission fields (can_add_tag, can_change_tag, can_delete_tag) will be None. * page (optional) - Page number (default: 1) * page_size (optional) - Number of items per page (default: 30). Ignored when there are fewer tags than specified by ?full_depth_threshold. diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index f296fb0b..df7e2ca5 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -147,7 +147,7 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c ) tag.save() - url = TAXONOMY_LIST_URL + '?include_perms' + url = TAXONOMY_LIST_URL if user_attr: user = getattr(self, user_attr) @@ -224,7 +224,7 @@ def test_list_taxonomy_empty(self, user_attr, expected_can_add) -> None: if language_taxonomy: language_taxonomy.delete() - url = TAXONOMY_LIST_URL + '?include_perms' + url = TAXONOMY_LIST_URL user = getattr(self, user_attr) self.client.force_authenticate(user=user) response = self.client.get(url, format="json") @@ -241,33 +241,26 @@ def test_list_taxonomy_empty(self, user_attr, expected_can_add) -> None: "can_add_taxonomy": expected_can_add, } - @ddt.data( - True, False - ) - def test_list_taxonomy_query_count(self, include_perms): + def test_list_taxonomy_query_count(self): """ Test how many queries are used when retrieving taxonomies and permissions """ api.create_taxonomy(name="T1", enabled=True) api.create_taxonomy(name="T2", enabled=True) - expected_perm = None url = TAXONOMY_LIST_URL - if include_perms: - url += '?include_perms' - expected_perm = False self.client.force_authenticate(user=self.user) with self.assertNumQueries(5): response = self.client.get(url) assert response.status_code == 200 - assert response.data["can_add_taxonomy"] == expected_perm + assert not response.data["can_add_taxonomy"] assert len(response.data["results"]) == 3 for taxonomy in response.data["results"]: - assert taxonomy["can_change_taxonomy"] == expected_perm - assert taxonomy["can_delete_taxonomy"] == expected_perm - assert taxonomy["can_tag_object"] == expected_perm + assert not taxonomy["can_change_taxonomy"] + assert not taxonomy["can_delete_taxonomy"] + assert not taxonomy["can_tag_object"] def test_list_invalid_page(self) -> None: url = TAXONOMY_LIST_URL @@ -296,6 +289,9 @@ 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, + can_change_taxonomy=False, + can_delete_taxonomy=False, + can_tag_object=False, ) @ddt.data( @@ -312,7 +308,7 @@ def test_detail_taxonomy( ): 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) + '?include_perms' + url = TAXONOMY_DETAIL_URL.format(pk=taxonomy.pk) if user_attr: user = getattr(self, user_attr) @@ -374,6 +370,9 @@ 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): + create_data["can_change_taxonomy"] = True + create_data["can_delete_taxonomy"] = True + create_data["can_tag_object"] = False check_taxonomy(response.data, response.data["id"], **create_data) url = TAXONOMY_DETAIL_URL.format(pk=response.data["id"]) @@ -427,7 +426,7 @@ def test_update_taxonomy(self, user_attr, expected_status): # If we were able to update the taxonomy, check if the name changed if status.is_success(expected_status): - response = self.client.get(url + '?include_perms') + response = self.client.get(url) check_taxonomy( response.data, response.data["id"], @@ -487,7 +486,7 @@ def test_patch_taxonomy(self, user_attr, expected_status): # If we were able to update the taxonomy, check if the name changed if status.is_success(expected_status): - response = self.client.get(url + '?include_perms') + response = self.client.get(url) check_taxonomy( response.data, response.data["id"], @@ -704,7 +703,7 @@ def test_retrieve_object_tags(self, user_attr, expected_status): api.tag_object(object_id=object_id, taxonomy=self.taxonomy, tags=["Mammalia", "Fungi"]) api.tag_object(object_id=object_id, taxonomy=self.user_taxonomy, tags=[self.user_1.username]) - url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) + '?include_perms' + url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) if user_attr: user = getattr(self, user_attr) @@ -758,7 +757,7 @@ def test_retrieve_object_tags(self, user_attr, expected_status): }, } - def prepare_for_sort_test(self, expected_perm=None) -> tuple[str, list[dict]]: + def prepare_for_sort_test(self, expected_perm=False) -> tuple[str, list[dict]]: """ Tag an object with tags from the "sort test" taxonomy """ @@ -825,7 +824,7 @@ def test_retrieve_object_tags_sorted(self): Test the sort order of the object tags retrieved from the get object tags API. """ - object_id, sort_test_applied_result = self.prepare_for_sort_test() + object_id, sort_test_applied_result = self.prepare_for_sort_test(expected_perm=True) url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) self.client.force_authenticate(user=self.user_1) @@ -834,18 +833,13 @@ def test_retrieve_object_tags_sorted(self): assert response.data[object_id]["taxonomies"][0]["name"] == "Sort Test" assert response.data[object_id]["taxonomies"][0]["tags"] == sort_test_applied_result - @ddt.data( - True, False - ) - def test_retrieve_object_tags_query_count(self, include_perms): + def test_retrieve_object_tags_query_count(self): """ Test how many queries are used when retrieving object tags and permissions """ - expected_perm = True if include_perms else None + expected_perm = True object_id, sort_test_applied_result = self.prepare_for_sort_test(expected_perm) url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) - if include_perms: - url += '?include_perms' self.client.force_authenticate(user=self.user_1) with self.assertNumQueries(1): @@ -901,13 +895,13 @@ def test_retrieve_object_tags_taxonomy_queryparam( { "name": "User Authors", "taxonomy_id": 3, - "can_tag_object": None, + "can_tag_object": True, "tags": [ { "value": "test_user_1", "lineage": ["test_user_1"], - "can_change_objecttag": None, - "can_delete_objecttag": None, + "can_change_objecttag": True, + "can_delete_objecttag": True, }, ], } @@ -1197,7 +1191,6 @@ def test_get_counts_invalid_spec(self): assert "Wildcard matches are only supported if the * is at the end." in str(result.content) -@ddt.ddt class TestTaxonomyTagsView(TestTaxonomyViewMixin): """ Tests the list/create/update/delete tags of taxonomy view @@ -1266,7 +1259,7 @@ def test_small_taxonomy_root(self): Test explicitly requesting only the root tags of a small taxonomy. """ self.client.force_authenticate(user=self.staff) - response = self.client.get(self.small_taxonomy_url + "?include_counts&include_perms") + response = self.client.get(self.small_taxonomy_url + "?include_counts") assert response.status_code == status.HTTP_200_OK data = response.data @@ -1428,29 +1421,22 @@ def test_small_search_shallow(self): " Euryarchaeida (children: 0)", ] - @ddt.data( - True, False - ) - def test_small_query_count(self, include_perms): + def test_small_query_count(self): """ Test how many queries are used when retrieving small taxonomies+tags and permissions """ - expected_perm = None url = f"{self.small_taxonomy_url}?search_term=eU" - if include_perms: - url += '&include_perms' - expected_perm = True self.client.force_authenticate(user=self.staff) with self.assertNumQueries(5): response = self.client.get(url) assert response.status_code == status.HTTP_200_OK - assert response.data["can_add_tag"] == expected_perm + assert response.data["can_add_tag"] assert len(response.data["results"]) == 3 for taxonomy in response.data["results"]: - assert taxonomy["can_change_tag"] == expected_perm - assert taxonomy["can_delete_tag"] == expected_perm + assert taxonomy["can_change_tag"] + assert taxonomy["can_delete_tag"] def test_empty_results(self): """ @@ -1474,24 +1460,14 @@ def assert_empty(url): assert_empty(f"{self.small_taxonomy_url}?search_term=eu&parent_tag=Euryarchaeida") assert_empty(f"{self.small_taxonomy_url}?search_term=eu&parent_tag=Euryarchaeida&full_depth_threshold=1000") - @ddt.data( - True, False, - ) - def test_large_taxonomy(self, include_perms): + def test_large_taxonomy(self): """ Test listing the tags in a large taxonomy (~7,000 tags). """ self._build_large_taxonomy() self.client.force_authenticate(user=self.staff) - expected_perm = None - expected_next_params = 'include_counts=&page=2' url = self.large_taxonomy_url + "?include_counts" - if include_perms: - url += "&include_perms" - expected_perm = True - expected_next_params = 'include_counts=&include_perms=&page=2' - with self.assertNumQueries(3): response = self.client.get(url) @@ -1525,19 +1501,19 @@ def test_large_taxonomy(self, include_perms): f"rest_api/v1/taxonomies/{self.large_taxonomy.id}" f"/tags/?parent_tag={quote_plus(results[0]['value'])}" ) - assert results[0].get("can_change_tag") == expected_perm - assert results[0].get("can_delete_tag") == expected_perm + assert results[0].get("can_change_tag") + assert results[0].get("can_delete_tag") # Checking pagination values assert data.get("next") == ( "http://testserver/tagging/" - f"rest_api/v1/taxonomies/{self.large_taxonomy.id}/tags/?{expected_next_params}" + f"rest_api/v1/taxonomies/{self.large_taxonomy.id}/tags/?include_counts=&page=2" ) assert data.get("previous") is None assert data.get("count") == self.root_tags_count assert data.get("num_pages") == 6 assert data.get("current_page") == 1 - assert data.get("can_add_tag") == expected_perm + assert data.get("can_add_tag") def test_next_page_large_taxonomy(self): self._build_large_taxonomy() From eaf9f67031e459e5e43953c901d56b8e7f8aac1a Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 23 Jan 2024 17:57:02 +1030 Subject: [PATCH 17/17] fix: addresses PR review * removes can_change_objecttag * replaces Optional[bool] with bool | None * adds comment about can_tag_object in tests --- .../core/tagging/rest_api/v1/serializers.py | 11 +++++------ tests/openedx_tagging/core/tagging/test_views.py | 7 ++----- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/serializers.py b/openedx_tagging/core/tagging/rest_api/v1/serializers.py index 15dc31fe..a3af0bbb 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/serializers.py +++ b/openedx_tagging/core/tagging/rest_api/v1/serializers.py @@ -3,7 +3,7 @@ """ from __future__ import annotations -from typing import Any, Optional, Type +from typing import Any, Type from urllib.parse import urlencode from rest_framework import serializers @@ -100,7 +100,7 @@ def to_representation(self, instance): def get_tags_count(self, instance): return instance.tag_set.count() - def get_can_tag_object(self, instance) -> Optional[bool]: + def get_can_tag_object(self, instance) -> bool | None: """ Returns True if the current request user may create object tags on this taxonomy. @@ -128,10 +128,9 @@ class ObjectTagMinimalSerializer(UserPermissionsSerializerMixin, serializers.Mod class Meta: model = ObjectTag - fields = ["value", "lineage", "can_change_objecttag", "can_delete_objecttag"] + fields = ["value", "lineage", "can_delete_objecttag"] lineage = serializers.ListField(child=serializers.CharField(), source="get_lineage", read_only=True) - can_change_objecttag = serializers.SerializerMethodField(method_name='get_can_change') can_delete_objecttag = serializers.SerializerMethodField(method_name='get_can_delete') @@ -249,7 +248,7 @@ def _model(self) -> Type: """ return Tag - def get_can_change_tag(self, _instance) -> Optional[bool]: + def get_can_change_tag(self, _instance) -> bool | None: """ Returns True if the current user is allowed to edit/change this Tag instance. @@ -258,7 +257,7 @@ def get_can_change_tag(self, _instance) -> Optional[bool]: """ return self.context.get('can_change_tag') - def get_can_delete_tag(self, _instance) -> Optional[bool]: + def get_can_delete_tag(self, _instance) -> bool | None: """ Returns True if the current user is allowed to delete this Tag instance. diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 9b0cc7ae..c2ccd320 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -187,6 +187,8 @@ def test_list_taxonomy(self, user_attr: str | None, expected_status: int, tags_c # Enabled taxonomy can be modified by taxonomy admins "can_change_taxonomy": is_admin, "can_delete_taxonomy": is_admin, + # can_tag_object is False because we default to not allowing users to tag arbitrary objects. + # But specific uses of this code (like content_tagging) will override this perm for their use cases. "can_tag_object": False, }, ] @@ -729,13 +731,11 @@ def test_retrieve_object_tags(self, user_attr, expected_status): { "value": "Mammalia", "lineage": ["Eukaryota", "Animalia", "Chordata", "Mammalia"], - "can_change_objecttag": True, "can_delete_objecttag": True, }, { "value": "Fungi", "lineage": ["Eukaryota", "Fungi"], - "can_change_objecttag": True, "can_delete_objecttag": True, }, ] @@ -748,7 +748,6 @@ def test_retrieve_object_tags(self, user_attr, expected_status): { "value": "test_user_1", "lineage": ["test_user_1"], - "can_change_objecttag": True, "can_delete_objecttag": True, }, ], @@ -781,7 +780,6 @@ def prepare_for_sort_test(self, expected_perm=False) -> tuple[str, list[dict]]: # Properties shared by all returned object tags shared_props = { - "can_change_objecttag": expected_perm, "can_delete_objecttag": expected_perm, } @@ -900,7 +898,6 @@ def test_retrieve_object_tags_taxonomy_queryparam( { "value": "test_user_1", "lineage": ["test_user_1"], - "can_change_objecttag": True, "can_delete_objecttag": True, }, ],