diff --git a/src/openedx_core/__init__.py b/src/openedx_core/__init__.py index 87e5a64b6..01d5f52bf 100644 --- a/src/openedx_core/__init__.py +++ b/src/openedx_core/__init__.py @@ -6,4 +6,4 @@ """ # The version for the entire repository -__version__ = "0.39.0" +__version__ = "0.39.1" diff --git a/src/openedx_tagging/rest_api/v1/exception_handlers.py b/src/openedx_tagging/rest_api/v1/exception_handlers.py new file mode 100644 index 000000000..58acaf22c --- /dev/null +++ b/src/openedx_tagging/rest_api/v1/exception_handlers.py @@ -0,0 +1,58 @@ +""" +Tagging REST API exception handling utilities. +""" + +import logging +import traceback + +from django.conf import settings +from django.http import Http404 +from rest_framework import status +from rest_framework.exceptions import APIException, PermissionDenied +from rest_framework.response import Response +from rest_framework.views import exception_handler + +log = logging.getLogger(__name__) + + +def _custom_exception_handler(exc: Exception, context: dict) -> Response | None: + """ + Return standard DRF errors for APIException and a generic 500 otherwise. + This exception handler should eventually be replaced by a more top-level + exception handler in the openedx-platform repo after the ADR for it is accepted: + https://github.com/openedx/openedx-platform/pull/38246 + """ + # For exceptions expected by DRF return the standard DRF error response: + # Instances of APIException, subclasses of APIException, Django's Http404 exception, + # and Django's PermissionDenied exception. + is_expected_exception = isinstance( + exc, (APIException, Http404, PermissionDenied) + ) + if is_expected_exception: + return exception_handler(exc, context) + + # DRF always calls exception handlers from within an `except:` block, so we can assume + # that `log.exception` will automatically insert those exception details and a stack trace. + log.exception("Unexpected exception while handling API request") + + if settings.DEBUG: + description_with_traceback = f"{exc.__class__.__name__}: {str(exc)}\n\nTraceback:\n{traceback.format_exc()}" + + return Response( + {"detail": description_with_traceback}, + status=status.HTTP_500_INTERNAL_SERVER_ERROR, + ) + + return Response( + {"detail": "An unexpected error occurred while processing the request."}, + status=status.HTTP_500_INTERNAL_SERVER_ERROR, + ) + + +class TaggingExceptionHandlerMixin: + """ + Scope custom exception handling to tagging API views only. + """ + + def get_exception_handler(self): + return _custom_exception_handler diff --git a/src/openedx_tagging/rest_api/v1/serializers.py b/src/openedx_tagging/rest_api/v1/serializers.py index a70f6d20f..9c5b62a06 100644 --- a/src/openedx_tagging/rest_api/v1/serializers.py +++ b/src/openedx_tagging/rest_api/v1/serializers.py @@ -13,6 +13,7 @@ from openedx_tagging.data import TagData from openedx_tagging.import_export.parsers import ParserFormat from openedx_tagging.models import ObjectTag, Tag, TagImportTask, Taxonomy +from openedx_tagging.models.utils import RESERVED_TAG_CHARS from openedx_tagging.rules import ObjectTagPermissionItem from ..utils import UserPermissionsHelper @@ -59,7 +60,7 @@ def _model(self) -> Type: @property def _request(self) -> Request: """ - Returns the current request from the serialize context. + Returns the current request from the serializer context. """ return self.context.get('request') # type: ignore[attr-defined] @@ -217,6 +218,27 @@ class ObjectTagUpdateBodySerializer(serializers.Serializer): # pylint: disable= tagsData = serializers.ListField(child=ObjectTagUpdateByTaxonomySerializer(), required=True) +def validate_tag_value(value, context): + """ + Validate this tag value is unique within the current taxonomy context and + does not contain forbidden characters. + """ + taxonomy_id = context.get("taxonomy_id") + if taxonomy_id is not None: + # Check if tag value already exists within this taxonomy. If so, raise a validation error. + queryset = Tag.objects.filter(taxonomy_id=taxonomy_id, value=value) + if queryset.exists(): + raise serializers.ValidationError( + f'Tag value "{value}" already exists in this taxonomy.', code='unique' + ) + + # validator checks there are no forbidden characters ">" or ";": + for char in value: + if char in RESERVED_TAG_CHARS: + raise serializers.ValidationError('Tag values cannot contain "\t" or ">" or ";" characters.') + return value + + class TagDataSerializer(UserPermissionsSerializerMixin, serializers.Serializer): # pylint: disable=abstract-method """ Serializer for TagData dicts. Also can serialize Tag instances. @@ -237,6 +259,12 @@ class TagDataSerializer(UserPermissionsSerializerMixin, serializers.Serializer): can_change_tag = serializers.SerializerMethodField() can_delete_tag = serializers.SerializerMethodField() + def validate_value(self, value): + """ + Runs validations for the tag value. + """ + return validate_tag_value(value, self.context) + def get_sub_tags_url(self, obj: TagData | Tag): """ Returns URL for the list of child tags of the current tag. @@ -303,6 +331,12 @@ class TaxonomyTagCreateBodySerializer(serializers.Serializer): # pylint: disabl parent_tag_value = serializers.CharField(required=False) external_id = serializers.CharField(required=False) + def validate_tag(self, value): + """ + Run validations for the tag value. + """ + return validate_tag_value(value, self.context) + class TaxonomyTagUpdateBodySerializer(serializers.Serializer): # pylint: disable=abstract-method """ @@ -312,6 +346,12 @@ class TaxonomyTagUpdateBodySerializer(serializers.Serializer): # pylint: disabl tag = serializers.CharField(required=True) updated_tag_value = serializers.CharField(required=True) + def validate_updated_tag_value(self, value): + """ + Run validations for the updated tag value. + """ + return validate_tag_value(value, self.context) + class TaxonomyTagDeleteBodySerializer(serializers.Serializer): # pylint: disable=abstract-method """ @@ -323,6 +363,25 @@ class TaxonomyTagDeleteBodySerializer(serializers.Serializer): # pylint: disabl ) with_subtags = serializers.BooleanField(required=False) + def validate_tags(self, tags_list): + """ + Make sure all tags are valid and exist before attempting deletion, to avoid partial deletes. + """ + # Iterate through the list and make one bulk request that checks whether every tag.value exists + taxonomy_id = self.context.get("taxonomy_id") + existing_tags = set( + Tag.objects.filter(taxonomy_id=taxonomy_id, value__in=tags_list) + .values_list("value", flat=True) + ) + missing_tags = set(tags_list) - existing_tags + + if missing_tags: + raise serializers.ValidationError( + f"Deletion aborted. The following tags do not exist and cannot be deleted:" + f" {', '.join(missing_tags)}" + ) + return tags_list + class TaxonomyImportBodySerializer(serializers.Serializer): # pylint: disable=abstract-method """ diff --git a/src/openedx_tagging/rest_api/v1/views.py b/src/openedx_tagging/rest_api/v1/views.py index 97761286f..14de82809 100644 --- a/src/openedx_tagging/rest_api/v1/views.py +++ b/src/openedx_tagging/rest_api/v1/views.py @@ -34,6 +34,7 @@ from ...rules import ObjectTagPermissionItem from ..paginators import MAX_FULL_DEPTH_THRESHOLD, DisabledTagsPagination, TagsPagination, TaxonomyPagination from ..utils import view_auth_classes +from .exception_handlers import TaggingExceptionHandlerMixin from .permissions import ObjectTagObjectPermissions, TaxonomyObjectPermissions, TaxonomyTagsObjectPermissions from .serializers import ( ObjectTagListQueryParamsSerializer, @@ -55,7 +56,7 @@ @view_auth_classes -class TaxonomyView(ModelViewSet): +class TaxonomyView(TaggingExceptionHandlerMixin, ModelViewSet): """ View to list, create, retrieve, update, delete, export or import Taxonomies. @@ -640,7 +641,7 @@ def retrieve(self, request, *args, **kwargs) -> Response: @view_auth_classes -class TaxonomyTagsView(ListAPIView, RetrieveUpdateDestroyAPIView): +class TaxonomyTagsView(TaggingExceptionHandlerMixin, ListAPIView, RetrieveUpdateDestroyAPIView): """ View to list/create/update/delete tags of a taxonomy. @@ -865,7 +866,8 @@ def post(self, request, *args, **kwargs): """ taxonomy = self.get_taxonomy() - body = TaxonomyTagCreateBodySerializer(data=request.data) + serializer_context = self.get_serializer_context() + body = TaxonomyTagCreateBodySerializer(data=request.data, context=serializer_context) body.is_valid(raise_exception=True) tag = body.data.get("tag") @@ -881,7 +883,6 @@ def post(self, request, *args, **kwargs): except ValueError as e: raise ValidationError(e) from e - serializer_context = self.get_serializer_context() return Response( self.serializer_class(new_tag, context=serializer_context).data, status=status.HTTP_201_CREATED @@ -894,7 +895,8 @@ def update(self, request, *args, **kwargs): """ taxonomy = self.get_taxonomy() - body = TaxonomyTagUpdateBodySerializer(data=request.data) + serializer_context = self.get_serializer_context() + body = TaxonomyTagUpdateBodySerializer(data=request.data, context=serializer_context) body.is_valid(raise_exception=True) tag = body.data.get("tag") @@ -907,7 +909,6 @@ def update(self, request, *args, **kwargs): except ValueError as e: raise ValidationError(e) from e - serializer_context = self.get_serializer_context() return Response( self.serializer_class(updated_tag, context=serializer_context).data, status=status.HTTP_200_OK @@ -921,7 +922,8 @@ def delete(self, request, *args, **kwargs): """ taxonomy = self.get_taxonomy() - body = TaxonomyTagDeleteBodySerializer(data=request.data) + serializer_context = self.get_serializer_context() + body = TaxonomyTagDeleteBodySerializer(data=request.data, context=serializer_context) body.is_valid(raise_exception=True) tags = body.data.get("tags") diff --git a/src/openedx_tagging/rest_api/v1/views_import.py b/src/openedx_tagging/rest_api/v1/views_import.py index 6c3a63398..14a615e31 100644 --- a/src/openedx_tagging/rest_api/v1/views_import.py +++ b/src/openedx_tagging/rest_api/v1/views_import.py @@ -9,8 +9,10 @@ from rest_framework.request import Request from rest_framework.views import APIView +from .exception_handlers import TaggingExceptionHandlerMixin -class TemplateView(APIView): + +class TemplateView(TaggingExceptionHandlerMixin, APIView): """ View which serves the static Taxonomy Import template files. diff --git a/tests/openedx_tagging/test_views.py b/tests/openedx_tagging/test_views.py index ac722b712..c87c3a95d 100644 --- a/tests/openedx_tagging/test_views.py +++ b/tests/openedx_tagging/test_views.py @@ -4,12 +4,15 @@ from __future__ import annotations import json +from unittest.mock import patch from urllib.parse import parse_qs, quote_plus, urlparse import ddt # type: ignore[import] import rules from django.contrib.auth import get_user_model from django.core.files.uploadedfile import SimpleUploadedFile +from django.db import IntegrityError +from django.test import override_settings from rest_framework import status from rest_framework.test import APITestCase @@ -2173,6 +2176,127 @@ def test_update_tag_in_taxonomy_with_different_methods(self): self.assertEqual(data.get("parent_value"), existing_tag.parent) self.assertEqual(data.get("external_id"), existing_tag.external_id) + def test_update_tag_with_duplicate_value(self): + self.client.force_authenticate(user=self.staff) + updated_tag_value = "Updated Tag" + + # Existing Tag that will be updated + existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() + + # Create another tag with the value that we will try to update to, to cause a duplicate value scenario + Tag.objects.create( + value=updated_tag_value, + taxonomy=self.small_taxonomy, + parent=None + ) + + update_data = { + "tag": existing_tag.value, + "updated_tag_value": updated_tag_value + } + + response = self.client.put( + self.small_taxonomy_url, update_data, format="json" + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + # Check that the error message indicates the duplicate value issue + assert "Tag value \"Updated Tag\" already exists in this taxonomy" in str(response.data) + + response = self.client.patch( + self.small_taxonomy_url, update_data, format="json" + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + # Check that the error message indicates the duplicate value issue + assert "Tag value \"Updated Tag\" already exists in this taxonomy" in str(response.data) + + def test_should_handle_unexpected_errors_gracefully(self): + """ + Test that if any unexpected error occurs during the processing of the request, + the API converts it to a generic 500 response without exposing sensitive info. + For example, if there is an IntegrityError, this is handled gracefully and returns a generic 500 error. + """ + self.client.force_authenticate(user=self.staff) + expected_error_message = "An unexpected error occurred while processing the request." + existing_tag = self.small_taxonomy.tag_set.filter(parent=None).first() + update_data = { + "tag": existing_tag.value, + "updated_tag_value": "Updated Tag" + } + create_data = { + "tag": "New Tag", + "parent_tag_value": existing_tag.value + } + delete_data = { + "tags": [existing_tag.value], + "with_subtags": True + } + + # Simulate a generic exception in a method used across all verbs in this view. + with patch("openedx_tagging.rest_api.v1.views.TaxonomyTagsView.get_taxonomy") as mock_get_taxonomy: + mock_get_taxonomy.side_effect = Exception("Unexpected error") + + response = self.client.get(self.small_taxonomy_url, {}, format="json") + assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + assert expected_error_message in str(response.data) + + response = self.client.put(self.small_taxonomy_url, update_data, format="json") + assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + assert expected_error_message in str(response.data) + + response = self.client.patch(self.small_taxonomy_url, update_data, format="json") + assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + assert expected_error_message in str(response.data) + + response = self.client.post(self.small_taxonomy_url, create_data, format="json") + assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + assert expected_error_message in str(response.data) + + response = self.client.delete(self.small_taxonomy_url, delete_data, format="json") + assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + assert expected_error_message in str(response.data) + + # Simulate an IntegrityError in the same shared method. + with patch("openedx_tagging.rest_api.v1.views.TaxonomyTagsView.get_taxonomy") as mock_get_taxonomy: + mock_get_taxonomy.side_effect = IntegrityError("Integrity error") + + response = self.client.get(self.small_taxonomy_url, {}, format="json") + assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + assert expected_error_message in str(response.data) + + response = self.client.put(self.small_taxonomy_url, update_data, format="json") + assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + assert expected_error_message in str(response.data) + + response = self.client.patch(self.small_taxonomy_url, update_data, format="json") + assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + assert expected_error_message in str(response.data) + + response = self.client.post(self.small_taxonomy_url, create_data, format="json") + assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + assert expected_error_message in str(response.data) + + response = self.client.delete(self.small_taxonomy_url, delete_data, format="json") + assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + assert expected_error_message in str(response.data) + + def test_passes_stack_trace_upwards_when_settings_debug_true(self): + """ + Test that when an unexpected error occurs, if we're in debug mode, the stack trace is included in the response + to help with debugging, instead of just a generic error message. + """ + self.client.force_authenticate(user=self.staff) + # simulate debug mode by patching the settings.DEBUG value to True + with override_settings(DEBUG=True): + with patch("openedx_tagging.rest_api.v1.views.TaxonomyTagsView.get_taxonomy") as mock_get_taxonomy: + mock_get_taxonomy.side_effect = Exception("Specific error message") + + response = self.client.get(self.small_taxonomy_url) + assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + assert "Specific error message" in str(response.data) + assert "get_taxonomy" in str(response.data) # Checking that the stack trace is included + def test_update_tag_in_taxonomy_reflects_changes_in_object_tags(self): self.client.force_authenticate(user=self.staff)