diff --git a/lms/djangoapps/discussion/rest_api/permissions.py b/lms/djangoapps/discussion/rest_api/permissions.py index 1346a4f76b..0e9cfb19ab 100644 --- a/lms/djangoapps/discussion/rest_api/permissions.py +++ b/lms/djangoapps/discussion/rest_api/permissions.py @@ -3,6 +3,18 @@ Discussion API permission logic """ from typing import Dict, Set, Union +from opaque_keys.edx.keys import CourseKey +from rest_framework import permissions + +from common.djangoapps.student.models import CourseEnrollment +from common.djangoapps.student.roles import ( + CourseInstructorRole, + CourseStaffRole, + GlobalStaff, +) +from lms.djangoapps.discussion.django_comment_client.utils import ( + has_discussion_privileges, +) from openedx.core.djangoapps.django_comment_common.comment_client.comment import Comment from openedx.core.djangoapps.django_comment_common.comment_client.thread import Thread @@ -118,3 +130,21 @@ def can_delete(cc_content, context): Return True if the requester can delete the given content, False otherwise """ return _is_author_or_privileged(cc_content, context) + + +class IsStaffOrCourseTeamOrEnrolled(permissions.BasePermission): + """ + Permission that checks to see if the user is allowed to post or + comment in the course. + """ + + def has_permission(self, request, view): + """Returns true if the user is enrolled or is staff.""" + course_key = CourseKey.from_string(view.kwargs.get('course_id')) + return ( + GlobalStaff().has_user(request.user) or + CourseStaffRole(course_key).has_user(request.user) or + CourseInstructorRole(course_key).has_user(request.user) or + CourseEnrollment.is_enrolled(request.user, course_key) or + has_discussion_privileges(request.user, course_key) + ) diff --git a/lms/djangoapps/discussion/rest_api/tests/test_views.py b/lms/djangoapps/discussion/rest_api/tests/test_views.py index b54b1235a1..106fb581fe 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_views.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_views.py @@ -11,10 +11,12 @@ from urllib.parse import urlparse import ddt import httpretty from django.urls import reverse +from django.core.files.uploadedfile import SimpleUploadedFile from opaque_keys.edx.keys import CourseKey from pytz import UTC from rest_framework.parsers import JSONParser from rest_framework.test import APIClient, APITestCase +from rest_framework import status from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -23,6 +25,7 @@ from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, chec from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.course_modes.tests.factories import CourseModeFactory from common.djangoapps.student.models import get_retired_username_by_username +from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole, GlobalStaff from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, SuperuserFactory, UserFactory from common.djangoapps.util.testing import PatchMediaTypeMixin, UrlResetMixin from common.test.utils import disable_signal @@ -133,6 +136,153 @@ class DiscussionAPIViewTestMixin(ForumsEnableMixin, CommentsServiceMockMixin, Ur self.test_basic() +@mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) +class UploadFileViewTest(ForumsEnableMixin, CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTestCase): + """ + Tests for UploadFileView. + """ + + @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) + def setUp(self): + super().setUp() + self.valid_file = { + "uploaded_file": SimpleUploadedFile( + "test.jpg", + b"test content", + content_type="image/jpeg", + ), + } + self.user = UserFactory.create(password="password") + self.course = CourseFactory.create(org='a', course='b', run='c', start=datetime.now(UTC)) + self.url = reverse("upload_file", kwargs={"course_id": str(self.course.id)}) + + def user_login(self): + """ + Authenticates the test client with the example user. + """ + self.client.login(username=self.user.username, password="password") + + def enroll_user_in_course(self): + """ + Makes the example user enrolled to the course. + """ + CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id) + + def assert_upload_success(self, response): + """ + Asserts that the upload response was successful and returned the + expected contents. + """ + assert response.status_code == status.HTTP_200_OK + assert response.content_type == "application/json" + response_data = json.loads(response.content) + assert "location" in response_data + + def test_file_upload_by_unauthenticated_user(self): + """ + Should fail if an unauthenticated user tries to upload a file. + """ + response = self.client.post(self.url, self.valid_file) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + def test_file_upload_by_unauthorized_user(self): + """ + Should fail if the user is not either staff or a student + enrolled in the course. + """ + self.user_login() + response = self.client.post(self.url, self.valid_file) + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_file_upload_by_enrolled_user(self): + """ + Should succeed when a valid file is uploaded by an authenticated + user who's enrolled in the course. + """ + self.user_login() + self.enroll_user_in_course() + response = self.client.post(self.url, self.valid_file) + self.assert_upload_success(response) + + def test_file_upload_by_global_staff(self): + """ + Should succeed when a valid file is uploaded by a global staff + member. + """ + self.user_login() + GlobalStaff().add_users(self.user) + response = self.client.post(self.url, self.valid_file) + self.assert_upload_success(response) + + def test_file_upload_by_instructor(self): + """ + Should succeed when a valid file is uploaded by a course instructor. + """ + self.user_login() + CourseInstructorRole(course_key=self.course.id).add_users(self.user) + response = self.client.post(self.url, self.valid_file) + self.assert_upload_success(response) + + def test_file_upload_by_course_staff(self): + """ + Should succeed when a valid file is uploaded by a course staff + member. + """ + self.user_login() + CourseStaffRole(course_key=self.course.id).add_users(self.user) + response = self.client.post(self.url, self.valid_file) + self.assert_upload_success(response) + + def test_file_upload_with_thread_key(self): + """ + Should contain the given thread_key in the uploaded file name. + """ + self.user_login() + self.enroll_user_in_course() + response = self.client.post(self.url, { + **self.valid_file, + "thread_key": "somethread", + }) + response_data = json.loads(response.content) + assert "/somethread/" in response_data["location"] + + def test_file_upload_with_invalid_file(self): + """ + Should fail if the uploaded file format is not allowed. + """ + self.user_login() + self.enroll_user_in_course() + invalid_file = { + "uploaded_file": SimpleUploadedFile( + "test.txt", + b"test content", + content_type="text/plain", + ), + } + response = self.client.post(self.url, invalid_file) + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_file_upload_with_invalid_course_id(self): + """ + Should fail if the course does not exist. + """ + self.user_login() + self.enroll_user_in_course() + url = reverse("upload_file", kwargs={"course_id": "d/e/f"}) + response = self.client.post(url, self.valid_file) + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_file_upload_with_no_data(self): + """ + Should fail when the user sends a request missing an + `uploaded_file` field. + """ + self.user_login() + self.enroll_user_in_course() + response = self.client.post(self.url, data={}) + assert response.status_code == status.HTTP_400_BAD_REQUEST + + @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) class CourseViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): """Tests for CourseView""" diff --git a/lms/djangoapps/discussion/rest_api/urls.py b/lms/djangoapps/discussion/rest_api/urls.py index ea9f6895a2..f915122ab7 100644 --- a/lms/djangoapps/discussion/rest_api/urls.py +++ b/lms/djangoapps/discussion/rest_api/urls.py @@ -16,7 +16,8 @@ from lms.djangoapps.discussion.rest_api.views import ( CourseView, ReplaceUsernamesView, RetireUserView, - ThreadViewSet + ThreadViewSet, + UploadFileView, ) ROUTER = SimpleRouter() @@ -31,6 +32,11 @@ urlpatterns = [ CourseDiscussionSettingsAPIView.as_view(), name="discussion_course_settings", ), + url( + fr"^v1/courses/{settings.COURSE_ID_PATTERN}/upload$", + UploadFileView.as_view(), + name="upload_file", + ), url( r"^v1/courses/{}/roles/(?P[A-Za-z0-9+ _-]+)/?$".format( settings.COURSE_ID_PATTERN diff --git a/lms/djangoapps/discussion/rest_api/views.py b/lms/djangoapps/discussion/rest_api/views.py index e040a7c2af..aa4a260575 100644 --- a/lms/djangoapps/discussion/rest_api/views.py +++ b/lms/djangoapps/discussion/rest_api/views.py @@ -4,19 +4,24 @@ Discussion API views import logging +import uuid +from django.core.exceptions import BadRequest from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser from opaque_keys.edx.keys import CourseKey from rest_framework import permissions, status +from rest_framework.authentication import SessionAuthentication from rest_framework.exceptions import ParseError, UnsupportedMediaType from rest_framework.parsers import JSONParser from rest_framework.response import Response from rest_framework.views import APIView from rest_framework.viewsets import ViewSet +from common.djangoapps.util.file import store_uploaded_file +from lms.djangoapps.discussion.django_comment_client import settings as cc_settings from lms.djangoapps.course_goals.models import UserActivity from lms.djangoapps.instructor.access import update_forum_role from openedx.core.djangoapps.discussions.serializers import DiscussionSettingsSerializer @@ -24,7 +29,7 @@ from openedx.core.djangoapps.django_comment_common import comment_client from openedx.core.djangoapps.django_comment_common.models import CourseDiscussionSettings, Role from openedx.core.djangoapps.user_api.accounts.permissions import CanReplaceUsername, CanRetireUser from openedx.core.djangoapps.user_api.models import UserRetirementStatus -from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser +from openedx.core.lib.api.authentication import BearerAuthentication, BearerAuthenticationAllowInactiveUser from openedx.core.lib.api.parsers import MergePatchParser from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes from xmodule.modulestore.django import modulestore @@ -53,6 +58,7 @@ from ..rest_api.serializers import ( DiscussionRolesListSerializer, DiscussionRolesSerializer, ) +from ..rest_api.permissions import IsStaffOrCourseTeamOrEnrolled log = logging.getLogger(__name__) @@ -94,6 +100,7 @@ class CourseView(DeveloperErrorViewMixin, APIView): * allow_anonymous_to_peers: A boolean which indicating whether posts anonymous to peers are allowed or not. """ + def get(self, request, course_id): """Implements the GET method as described in the class docstring.""" course_key = CourseKey.from_string(course_id) # TODO: which class is right? @@ -129,6 +136,7 @@ class CourseTopicsView(DeveloperErrorViewMixin, APIView): * non_courseware_topics: The list of topic trees that are not linked to courseware. Items are of the same format as in courseware_topics. """ + def get(self, request, course_id): """ Implements the GET method as described in the class docstring. @@ -607,6 +615,79 @@ class CommentViewSet(DeveloperErrorViewMixin, ViewSet): return Response(update_comment(request, comment_id, request.data)) +class UploadFileView(DeveloperErrorViewMixin, APIView): + """ + **Use Cases** + + Upload a file to be attached to a thread or comment. + + **URL Parameters** + + * course_id: + The ID of the course where this thread or comment belongs. + + **POST Upload File Parameters** + + * thread_key: + If the upload belongs to a comment, refer to the parent + `thread_id`, otherwise it should be `"root"`. + + **Example Requests**: + POST /api/discussion/v1/courses/{course_id}/upload/ + Content-Type: multipart/form-data; boundary=--Boundary + + ----Boundary + Content-Disposition: form-data; name="thread_key" + + + ----Boundary + Content-Disposition: form-data; name="uploaded_file"; filename="." + Content-Type: + + + ----Boundary-- + + **Response Values** + + * location: The URL to access the uploaded file. + """ + + authentication_classes = ( + JwtAuthentication, + BearerAuthentication, + SessionAuthentication, + ) + permission_classes = ( + permissions.IsAuthenticated, + IsStaffOrCourseTeamOrEnrolled, + ) + + def post(self, request, course_id): + """ + Handles a file upload. + """ + thread_key = request.POST.get("thread_key", "root") + unique_file_name = f"{course_id}/{thread_key}/{uuid.uuid4()}" + try: + file_storage, stored_file_name = store_uploaded_file( + request, "uploaded_file", cc_settings.ALLOWED_UPLOAD_FILE_TYPES, + unique_file_name, max_file_size=cc_settings.MAX_UPLOAD_FILE_SIZE, + ) + except ValueError: + raise BadRequest("no `uploaded_file` was provided") # lint-amnesty, pylint: disable=raise-missing-from + + file_absolute_url = file_storage.url(stored_file_name) + + # this is a no-op in production, but is required in development, + # since the filesystem storage returns the path without a base_url + file_absolute_url = request.build_absolute_uri(file_absolute_url) + + return Response( + {"location": file_absolute_url}, + content_type="application/json", + ) + + class RetireUserView(APIView): """ **Use Cases**