From 4676cca94a398f2bdec25fb4e0cbe8f87cf106f6 Mon Sep 17 00:00:00 2001 From: Nicolas Clerc Date: Fri, 21 Jul 2023 17:00:43 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=82(backend)=20use=20playlist=20token?= =?UTF-8?q?=20in=20markdown=20apis?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As we are now using playlist tokens, it has to be allowed on apis. --- src/backend/marsha/markdown/api.py | 26 ++++++++--- src/backend/marsha/markdown/permissions.py | 44 +++++-------------- src/backend/marsha/markdown/serializers.py | 9 +--- .../api/markdown_documents/test_create.py | 6 ++- .../api/markdown_documents/test_delete.py | 6 ++- .../tests/api/markdown_documents/test_list.py | 6 ++- .../markdown_documents/test_render_latex.py | 6 ++- .../api/markdown_documents/test_retrieve.py | 8 ++-- .../api/markdown_documents/test_update.py | 8 ++-- .../test_update_translations.py | 6 ++- .../tests/api/markdown_images/test_create.py | 10 +++-- .../tests/api/markdown_images/test_delete.py | 8 ++-- .../markdown_images/test_initiate_upload.py | 10 +++-- .../api/markdown_images/test_retrieve.py | 18 +++++--- 14 files changed, 96 insertions(+), 75 deletions(-) diff --git a/src/backend/marsha/markdown/api.py b/src/backend/marsha/markdown/api.py index 4eca605f91..733249aac6 100644 --- a/src/backend/marsha/markdown/api.py +++ b/src/backend/marsha/markdown/api.py @@ -23,6 +23,22 @@ from .utils.converter import LatexConversionException, render_latex_to_image +class ObjectMarkdownDocumentRelatedMixin: + """ + Get the related markdown document id contained in resource. + + It exposes a function used to get the related markdown document. + It is also useful to avoid URL crafting (when the url markdown_document_id doesn't + match token resource markdown document id). + """ + + def get_related_markdown_document_id(self): + """Get the related markdown document ID from the request.""" + + # The video ID in the URL is mandatory. + return self.kwargs.get("markdown_document_id") + + class MarkdownDocumentFilter(django_filters.FilterSet): """Filter for file depository.""" @@ -57,7 +73,7 @@ class MarkdownDocumentViewSet( permission_classes = [ ( - core_permissions.IsTokenResourceRouteObject + core_permissions.IsPlaylistToken & (core_permissions.IsTokenInstructor | core_permissions.IsTokenAdmin) ) | markdown_permissions.IsMarkdownDocumentPlaylistOrOrganizationAdmin @@ -272,6 +288,7 @@ class MarkdownImageViewSet( APIViewMixin, ObjectPkMixin, ObjectRelatedMixin, + ObjectMarkdownDocumentRelatedMixin, mixins.CreateModelMixin, mixins.DestroyModelMixin, mixins.RetrieveModelMixin, @@ -293,9 +310,7 @@ def get_permissions(self): else: permission_classes = [ markdown_permissions.IsTokenResourceRouteObjectRelatedMarkdownDocument - & core_permissions.IsTokenInstructor - | markdown_permissions.IsTokenResourceRouteObjectRelatedMarkdownDocument - & core_permissions.IsTokenAdmin + & (core_permissions.IsTokenInstructor | core_permissions.IsTokenAdmin) | IsRelatedMarkdownDocumentPlaylistOrOrganizationAdmin ] return [permission() for permission in permission_classes] @@ -306,7 +321,8 @@ def get_queryset(self): """ if self.request.resource: return MarkdownImage.objects.filter( - markdown_document__id=self.request.resource.id, + markdown_document__id=self.get_related_markdown_document_id(), + markdown_document__playlist__id=self.request.resource.id, ) return MarkdownImage.objects.all() diff --git a/src/backend/marsha/markdown/permissions.py b/src/backend/marsha/markdown/permissions.py index 95661e2574..85f7fae803 100644 --- a/src/backend/marsha/markdown/permissions.py +++ b/src/backend/marsha/markdown/permissions.py @@ -4,6 +4,7 @@ from rest_framework import permissions from marsha.core import models +from marsha.markdown.models import MarkdownDocument def _is_organization_admin(user_id, markdown_document_id): @@ -32,24 +33,23 @@ def _is_playlist_admin(user_id, markdown_document_id): ).exists() -class IsTokenResourceRouteObjectRelatedResource(permissions.BasePermission): +class IsTokenResourceRouteObjectRelatedMarkdownDocument(permissions.BasePermission): """ - Base permission class for JWT Tokens related to a resource object linked to a resource. + Base permission class for JWT Tokens related to a resource object linked to a + Markdown document. - These permissions grant access to users authenticated with a resource JWT token built from a - resource. + These permissions grants access to users authenticated with a JWT token built from a + resource ie related to a TokenUser as defined in `rest_framework_simplejwt`. """ - linked_resource_attribute = "" - def has_permission(self, request, view): """ - Allow the request if the JWT resource matches the resource - related to the object in the url. + Allow the request if the JWT resource matches the Markdown document related to the object + in the url. Parameters ---------- - request : Type[rest_framework.request.Request] + request : Type[django.http.request.HttpRequest] The request that holds the authenticated user view : Type[rest_framework.viewsets or rest_framework.views] The API view for which permissions are being checked @@ -62,29 +62,9 @@ def has_permission(self, request, view): if not request.resource: return False - try: - return ( - str( - getattr( - view.get_related_object(), - self.linked_resource_attribute, - ).id - ) - == request.resource.id - ) - except ObjectDoesNotExist: - return False - - -class IsTokenResourceRouteObjectRelatedMarkdownDocument( - IsTokenResourceRouteObjectRelatedResource -): - """ - Base permission class for JWT Tokens related to a resource object - linked to a Markdown document. - """ - - linked_resource_attribute = "markdown_document" + return MarkdownDocument.objects.filter( + pk=view.get_related_markdown_document_id(), playlist_id=request.resource.id + ).exists() class IsMarkdownDocumentPlaylistOrOrganizationAdmin(permissions.BasePermission): diff --git a/src/backend/marsha/markdown/serializers.py b/src/backend/marsha/markdown/serializers.py index 9e1725ba35..1f3bac005a 100644 --- a/src/backend/marsha/markdown/serializers.py +++ b/src/backend/marsha/markdown/serializers.py @@ -64,14 +64,9 @@ def create(self, validated_data): The "validated_data" dictionary is returned after modification. """ - # resource here is a Markdown document - resource = self.context["request"].resource - markdown_document_id = self.context["request"].data.get("markdown_document") + markdown_document_id = self.context["view"].get_related_markdown_document_id() if not validated_data.get("markdown_document_id"): - if resource: - validated_data["markdown_document_id"] = resource.id - elif markdown_document_id: - validated_data["markdown_document_id"] = markdown_document_id + validated_data["markdown_document_id"] = markdown_document_id return super().create(validated_data) diff --git a/src/backend/marsha/markdown/tests/api/markdown_documents/test_create.py b/src/backend/marsha/markdown/tests/api/markdown_documents/test_create.py index 0bccad4ead..40a44a0b28 100644 --- a/src/backend/marsha/markdown/tests/api/markdown_documents/test_create.py +++ b/src/backend/marsha/markdown/tests/api/markdown_documents/test_create.py @@ -38,7 +38,7 @@ def test_api_document_create_student(self): markdown_document = MarkdownDocumentFactory() jwt_token = StudentLtiTokenFactory( - resource=markdown_document, + resource=markdown_document.playlist, permissions__can_update=True, ) @@ -60,7 +60,9 @@ def test_api_document_create_instructor(self): """An instructor should not be able to create a Markdown document.""" markdown_document = MarkdownDocumentFactory() - jwt_token = InstructorOrAdminLtiTokenFactory(resource=markdown_document) + jwt_token = InstructorOrAdminLtiTokenFactory( + resource=markdown_document.playlist + ) response = self.client.post( "/api/markdown-documents/", diff --git a/src/backend/marsha/markdown/tests/api/markdown_documents/test_delete.py b/src/backend/marsha/markdown/tests/api/markdown_documents/test_delete.py index c5454e6088..09a597ae78 100644 --- a/src/backend/marsha/markdown/tests/api/markdown_documents/test_delete.py +++ b/src/backend/marsha/markdown/tests/api/markdown_documents/test_delete.py @@ -38,7 +38,7 @@ def test_api_document_delete_student(self): markdown_document = MarkdownDocumentFactory() jwt_token = StudentLtiTokenFactory( - resource=markdown_document, + resource=markdown_document.playlist, permissions__can_update=True, ) @@ -52,7 +52,9 @@ def test_api_document_delete_instructor(self): """An instructor should not be able to create a Markdown document.""" markdown_document = MarkdownDocumentFactory() - jwt_token = InstructorOrAdminLtiTokenFactory(resource=markdown_document) + jwt_token = InstructorOrAdminLtiTokenFactory( + resource=markdown_document.playlist + ) response = self.client.delete( f"/api/markdown-documents/{markdown_document.pk}/", diff --git a/src/backend/marsha/markdown/tests/api/markdown_documents/test_list.py b/src/backend/marsha/markdown/tests/api/markdown_documents/test_list.py index 308ff8195c..b2826d860c 100644 --- a/src/backend/marsha/markdown/tests/api/markdown_documents/test_list.py +++ b/src/backend/marsha/markdown/tests/api/markdown_documents/test_list.py @@ -36,7 +36,7 @@ def test_api_document_fetch_list_student(self): markdown_document = MarkdownDocumentFactory() jwt_token = StudentLtiTokenFactory( - resource=markdown_document, + resource=markdown_document.playlist, permissions__can_update=True, ) @@ -49,7 +49,9 @@ def test_api_document_fetch_list_instructor(self): """An instrustor should not be able to fetch a Markdown document list.""" markdown_document = MarkdownDocumentFactory() - jwt_token = InstructorOrAdminLtiTokenFactory(resource=markdown_document) + jwt_token = InstructorOrAdminLtiTokenFactory( + resource=markdown_document.playlist + ) response = self.client.get( "/api/markdown-documents/", HTTP_AUTHORIZATION=f"Bearer {jwt_token}" diff --git a/src/backend/marsha/markdown/tests/api/markdown_documents/test_render_latex.py b/src/backend/marsha/markdown/tests/api/markdown_documents/test_render_latex.py index c41ccffcbe..9bee8a7969 100644 --- a/src/backend/marsha/markdown/tests/api/markdown_documents/test_render_latex.py +++ b/src/backend/marsha/markdown/tests/api/markdown_documents/test_render_latex.py @@ -32,7 +32,7 @@ def test_api_document_render_latex_student(self): markdown_document = MarkdownDocumentFactory() jwt_token = StudentLtiTokenFactory( - resource=markdown_document, + resource=markdown_document.playlist, permissions__can_update=True, ) @@ -48,7 +48,9 @@ def test_api_document_render_latex_instructor(self): """An instructor should be able to render LaTeX content.""" markdown_document = MarkdownDocumentFactory(is_draft=True) - jwt_token = InstructorOrAdminLtiTokenFactory(resource=markdown_document) + jwt_token = InstructorOrAdminLtiTokenFactory( + resource=markdown_document.playlist + ) response = self.client.post( f"/api/markdown-documents/{markdown_document.pk}/latex-rendering/", diff --git a/src/backend/marsha/markdown/tests/api/markdown_documents/test_retrieve.py b/src/backend/marsha/markdown/tests/api/markdown_documents/test_retrieve.py index 0a09c55f09..c9dbc8f740 100644 --- a/src/backend/marsha/markdown/tests/api/markdown_documents/test_retrieve.py +++ b/src/backend/marsha/markdown/tests/api/markdown_documents/test_retrieve.py @@ -43,7 +43,7 @@ def test_api_document_fetch_student(self): markdown_document = MarkdownDocumentFactory() jwt_token = StudentLtiTokenFactory( - resource=markdown_document, + resource=markdown_document.playlist, permissions__can_update=True, ) @@ -69,7 +69,9 @@ def test_api_document_fetch_instructor(self): translations__rendered_content="

Heading1

\n

Some content

", ) - jwt_token = InstructorOrAdminLtiTokenFactory(resource=markdown_document) + jwt_token = InstructorOrAdminLtiTokenFactory( + resource=markdown_document.playlist + ) response = self.client.get( f"/api/markdown-documents/{markdown_document.pk}/", @@ -130,7 +132,7 @@ def test_api_document_fetch_instructor_read_only(self): markdown_document = MarkdownDocumentFactory() jwt_token = InstructorOrAdminLtiTokenFactory( - resource=markdown_document, + resource=markdown_document.playlist, permissions__can_update=False, ) diff --git a/src/backend/marsha/markdown/tests/api/markdown_documents/test_update.py b/src/backend/marsha/markdown/tests/api/markdown_documents/test_update.py index 7a73c9b581..cb2dd9d3cc 100644 --- a/src/backend/marsha/markdown/tests/api/markdown_documents/test_update.py +++ b/src/backend/marsha/markdown/tests/api/markdown_documents/test_update.py @@ -48,7 +48,7 @@ def test_api_document_update_student(self): markdown_document = MarkdownDocumentFactory() jwt_token = StudentLtiTokenFactory( - resource=markdown_document, + resource=markdown_document.playlist, permissions__can_update=True, ) data = {"title": "new title"} @@ -66,7 +66,7 @@ def test_api_document_update_instructor_read_only(self): markdown_document = MarkdownDocumentFactory() jwt_token = InstructorOrAdminLtiTokenFactory( - resource=markdown_document, + resource=markdown_document.playlist, permissions__can_update=False, ) data = {"title": "new title"} @@ -99,7 +99,9 @@ def test_api_document_update_instructor(self): """An instructor should be able to update a Markdown document.""" markdown_document = MarkdownDocumentFactory(is_draft=True) - jwt_token = InstructorOrAdminLtiTokenFactory(resource=markdown_document) + jwt_token = InstructorOrAdminLtiTokenFactory( + resource=markdown_document.playlist + ) data = {"is_draft": False} diff --git a/src/backend/marsha/markdown/tests/api/markdown_documents/test_update_translations.py b/src/backend/marsha/markdown/tests/api/markdown_documents/test_update_translations.py index 24979934de..7689c350c7 100644 --- a/src/backend/marsha/markdown/tests/api/markdown_documents/test_update_translations.py +++ b/src/backend/marsha/markdown/tests/api/markdown_documents/test_update_translations.py @@ -30,7 +30,7 @@ def test_api_document_translation_update_student(self): markdown_document = MarkdownDocumentFactory() jwt_token = StudentLtiTokenFactory( - resource=markdown_document, + resource=markdown_document.playlist, permissions__can_update=True, ) @@ -53,7 +53,9 @@ def test_api_document_translation_update_instructor(self): """An instructor should be able to update a Markdown document translated content.""" markdown_document = MarkdownDocumentFactory(is_draft=True) - jwt_token = InstructorOrAdminLtiTokenFactory(resource=markdown_document) + jwt_token = InstructorOrAdminLtiTokenFactory( + resource=markdown_document.playlist + ) data = { "language_code": "en", diff --git a/src/backend/marsha/markdown/tests/api/markdown_images/test_create.py b/src/backend/marsha/markdown/tests/api/markdown_images/test_create.py index 602ba6b8f1..c5e8959694 100644 --- a/src/backend/marsha/markdown/tests/api/markdown_images/test_create.py +++ b/src/backend/marsha/markdown/tests/api/markdown_images/test_create.py @@ -35,7 +35,7 @@ def test_api_markdown_image_create_student(self): """Student users should not be able to create a Markdown image.""" markdown_document = MarkdownDocumentFactory() - jwt_token = StudentLtiTokenFactory(resource=markdown_document) + jwt_token = StudentLtiTokenFactory(resource=markdown_document.playlist) response = self.client.post( f"/api/markdown-documents/{markdown_document.id}/markdown-images/", @@ -48,7 +48,9 @@ def test_api_markdown_image_create_instructor(self): """Instructors users should be able to create a Markdown image.""" markdown_document = MarkdownDocumentFactory() - jwt_token = InstructorOrAdminLtiTokenFactory(resource=markdown_document) + jwt_token = InstructorOrAdminLtiTokenFactory( + resource=markdown_document.playlist + ) response = self.client.post( f"/api/markdown-documents/{markdown_document.id}/markdown-images/", @@ -80,7 +82,9 @@ def test_api_markdown_image_create_already_existing_instructor(self): markdown_document = MarkdownDocumentFactory() MarkdownImageFactory(markdown_document=markdown_document) - jwt_token = InstructorOrAdminLtiTokenFactory(resource=markdown_document) + jwt_token = InstructorOrAdminLtiTokenFactory( + resource=markdown_document.playlist + ) response = self.client.post( f"/api/markdown-documents/{markdown_document.id}/markdown-images/", diff --git a/src/backend/marsha/markdown/tests/api/markdown_images/test_delete.py b/src/backend/marsha/markdown/tests/api/markdown_images/test_delete.py index b2832d9c80..deeaa9f402 100644 --- a/src/backend/marsha/markdown/tests/api/markdown_images/test_delete.py +++ b/src/backend/marsha/markdown/tests/api/markdown_images/test_delete.py @@ -35,7 +35,9 @@ def test_api_markdown_image_delete_student(self): """Student users should not be able to delete a Markdown image.""" markdown_image = MarkdownImageFactory() - jwt_token = StudentLtiTokenFactory(resource=markdown_image.markdown_document) + jwt_token = StudentLtiTokenFactory( + resource=markdown_image.markdown_document.playlist + ) response = self.client.delete( f"/api/markdown-documents/{markdown_image.markdown_document.id}" @@ -49,7 +51,7 @@ def test_api_markdown_image_delete_instructor(self): markdown_image = MarkdownImageFactory() jwt_token = InstructorOrAdminLtiTokenFactory( - resource=markdown_image.markdown_document, + resource=markdown_image.markdown_document.playlist, ) self.assertEqual(MarkdownImage.objects.count(), 1) @@ -84,7 +86,7 @@ def test_api_markdown_image_delete_instructor_in_read_only(self): markdown_image = MarkdownImageFactory() jwt_token = InstructorOrAdminLtiTokenFactory( - resource=markdown_image.markdown_document, + resource=markdown_image.markdown_document.playlist, permissions__can_update=False, ) diff --git a/src/backend/marsha/markdown/tests/api/markdown_images/test_initiate_upload.py b/src/backend/marsha/markdown/tests/api/markdown_images/test_initiate_upload.py index efb983e280..1bbdc4a255 100644 --- a/src/backend/marsha/markdown/tests/api/markdown_images/test_initiate_upload.py +++ b/src/backend/marsha/markdown/tests/api/markdown_images/test_initiate_upload.py @@ -38,7 +38,9 @@ def test_api_markdown_image_initiate_upload_anonymous(self): def test_api_markdown_image_initiate_upload_student(self): """Student users should not be allowed to initiate an upload.""" markdown_image = MarkdownImageFactory() - jwt_token = StudentLtiTokenFactory(resource=markdown_image.markdown_document) + jwt_token = StudentLtiTokenFactory( + resource=markdown_image.markdown_document.playlist + ) response = self.client.post( f"/api/markdown-documents/{markdown_image.markdown_document.id}" @@ -57,7 +59,9 @@ def test_api_markdown_image_initiate_upload_instructor(self): markdown_document=markdown_document, upload_state="ready", ) - jwt_token = InstructorOrAdminLtiTokenFactory(resource=markdown_document) + jwt_token = InstructorOrAdminLtiTokenFactory( + resource=markdown_document.playlist + ) # Get the upload policy for this Markdown image # It should generate a key file with the Unix timestamp of the present time @@ -114,7 +118,7 @@ def test_api_markdown_image_initiate_upload_instructor_read_only(self): markdown_image = MarkdownImageFactory() jwt_token = InstructorOrAdminLtiTokenFactory( - resource=markdown_image.markdown_document, + resource=markdown_image.markdown_document.playlist, permissions__can_update=False, ) diff --git a/src/backend/marsha/markdown/tests/api/markdown_images/test_retrieve.py b/src/backend/marsha/markdown/tests/api/markdown_images/test_retrieve.py index eccfed2b03..21dde7b8d8 100644 --- a/src/backend/marsha/markdown/tests/api/markdown_images/test_retrieve.py +++ b/src/backend/marsha/markdown/tests/api/markdown_images/test_retrieve.py @@ -40,7 +40,9 @@ def test_api_markdown_image_read_detail_student(self): """Students users should not be allowed to read a Markdown image detail.""" markdown_image = MarkdownImageFactory() - jwt_token = StudentLtiTokenFactory(resource=markdown_image.markdown_document) + jwt_token = StudentLtiTokenFactory( + resource=markdown_image.markdown_document.playlist + ) response = self.client.get( f"/api/markdown-documents/{markdown_image.markdown_document.id}" @@ -55,7 +57,7 @@ def test_api_markdown_image_instructor_read_detail_in_read_only(self): markdown_image = MarkdownImageFactory() jwt_token = InstructorOrAdminLtiTokenFactory( - resource=markdown_image.markdown_document, + resource=markdown_image.markdown_document.playlist, permissions__can_update=False, ) @@ -79,7 +81,9 @@ def test_api_markdown_image_read_detail_token_user(self): upload_state="pending", ) - jwt_token = InstructorOrAdminLtiTokenFactory(resource=markdown_document) + jwt_token = InstructorOrAdminLtiTokenFactory( + resource=markdown_document.playlist + ) response = self.client.get( f"/api/markdown-documents/{markdown_image.markdown_document.id}" @@ -108,7 +112,7 @@ def test_api_markdown_image_administrator_read_detail_in_read_only(self): markdown_image = MarkdownImageFactory() jwt_token = InstructorOrAdminLtiTokenFactory( - resource=markdown_image.markdown_document, + resource=markdown_image.markdown_document.playlist, permissions__can_update=False, ) @@ -133,7 +137,7 @@ def test_api_markdown_image_read_detail_admin_user(self): ) jwt_token = InstructorOrAdminLtiTokenFactory( - resource=markdown_document, + resource=markdown_document.playlist, roles=["administrator"], ) @@ -172,7 +176,9 @@ def test_api_markdown_image_read_ready_markdown_image(self): extension="gif", ) - jwt_token = InstructorOrAdminLtiTokenFactory(resource=markdown_document) + jwt_token = InstructorOrAdminLtiTokenFactory( + resource=markdown_document.playlist + ) response = self.client.get( f"/api/markdown-documents/{markdown_image.markdown_document.id}"