diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index db09fee31b..c307140552 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/lms/djangoapps/discussion_api/api.py @@ -17,6 +17,7 @@ from opaque_keys.edx.locator import CourseKey from courseware.courses import get_course_with_access from discussion_api.forms import CommentActionsForm, ThreadActionsForm from discussion_api.pagination import get_paginated_data +from discussion_api.permissions import can_delete, get_editable_fields from discussion_api.serializers import CommentSerializer, ThreadSerializer, get_context from django_comment_client.base.views import ( THREAD_CREATED_EVENT_NAME, @@ -91,19 +92,6 @@ def _get_comment_and_context(request, comment_id): raise Http404 -def _is_user_author_or_privileged(cc_content, context): - """ - Check if the user is the author of a content object or a privileged user. - - Returns: - Boolean - """ - return ( - context["is_requester_privileged"] or - context["cc_requester"]["id"] == cc_content["user_id"] - ) - - def get_thread_list_url(request, course_key, topic_id_list=None): """ Returns the URL for the thread_list_url field, given a list of topic_ids @@ -352,6 +340,21 @@ def get_comment_list(request, thread_id, endorsed, page, page_size): return get_paginated_data(request, results, page, num_pages) +def _check_editable_fields(cc_content, data, context): + """ + Raise ValidationError if the given update data contains a field that is not + in editable_fields. + """ + editable_fields = get_editable_fields(cc_content, context) + non_editable_errors = { + field: ["This field is not editable."] + for field in data.keys() + if field not in editable_fields + } + if non_editable_errors: + raise ValidationError(non_editable_errors) + + def _do_extra_actions(api_content, cc_content, request_fields, actions_form, context): """ Perform any necessary additional actions related to content creation or @@ -462,35 +465,7 @@ def create_comment(request, comment_data): get_comment_created_event_data(cc_comment, cc_thread["commentable_id"], followed=False) ) - return serializer.data - - -_THREAD_EDITABLE_BY_ANY = {"following", "voted"} -_THREAD_EDITABLE_BY_AUTHOR = {"topic_id", "type", "title", "raw_body"} | _THREAD_EDITABLE_BY_ANY - - -def _get_thread_editable_fields(cc_thread, context): - """ - Get the list of editable fields for the given thread in the given context - """ - if _is_user_author_or_privileged(cc_thread, context): - return _THREAD_EDITABLE_BY_AUTHOR - else: - return _THREAD_EDITABLE_BY_ANY - - -def _check_editable_fields(editable_fields, update_data): - """ - Raise ValidationError if the given update data contains a field that is not - in editable_fields. - """ - non_editable_errors = { - field: ["This field is not editable."] - for field in update_data.keys() - if field not in editable_fields - } - if non_editable_errors: - raise ValidationError(non_editable_errors) + return api_comment def update_thread(request, thread_id, update_data): @@ -512,8 +487,7 @@ def update_thread(request, thread_id, update_data): detail. """ cc_thread, context = _get_thread_and_context(request, thread_id) - editable_fields = _get_thread_editable_fields(cc_thread, context) - _check_editable_fields(editable_fields, update_data) + _check_editable_fields(cc_thread, update_data, context) serializer = ThreadSerializer(cc_thread, data=update_data, partial=True, context=context) actions_form = ThreadActionsForm(update_data) if not (serializer.is_valid() and actions_form.is_valid()): @@ -526,22 +500,6 @@ def update_thread(request, thread_id, update_data): return api_thread -_COMMENT_EDITABLE_BY_AUTHOR = {"raw_body"} -_COMMENT_EDITABLE_BY_THREAD_AUTHOR = {"endorsed"} - - -def _get_comment_editable_fields(cc_comment, context): - """ - Get the list of editable fields for the given comment in the given context - """ - ret = {"voted"} - if _is_user_author_or_privileged(cc_comment, context): - ret |= _COMMENT_EDITABLE_BY_AUTHOR - if _is_user_author_or_privileged(context["thread"], context): - ret |= _COMMENT_EDITABLE_BY_THREAD_AUTHOR - return ret - - def update_comment(request, comment_id, update_data): """ Update a comment. @@ -572,13 +530,12 @@ def update_comment(request, comment_id, update_data): is empty or thread_id is included) """ cc_comment, context = _get_comment_and_context(request, comment_id) - editable_fields = _get_comment_editable_fields(cc_comment, context) - _check_editable_fields(editable_fields, update_data) + _check_editable_fields(cc_comment, update_data, context) serializer = CommentSerializer(cc_comment, data=update_data, partial=True, context=context) actions_form = CommentActionsForm(update_data) if not (serializer.is_valid() and actions_form.is_valid()): raise ValidationError(dict(serializer.errors.items() + actions_form.errors.items())) - # Only save thread object if some of the edited fields are in the thread data, not extra actions + # Only save comment object if some of the edited fields are in the comment data, not extra actions if set(update_data) - set(actions_form.fields): serializer.save() api_comment = serializer.data @@ -603,7 +560,7 @@ def delete_thread(request, thread_id): """ cc_thread, context = _get_thread_and_context(request, thread_id) - if _is_user_author_or_privileged(cc_thread, context): + if can_delete(cc_thread, context): cc_thread.delete() else: raise PermissionDenied @@ -626,7 +583,7 @@ def delete_comment(request, comment_id): """ cc_comment, context = _get_comment_and_context(request, comment_id) - if _is_user_author_or_privileged(cc_comment, context): + if can_delete(cc_comment, context): cc_comment.delete() else: raise PermissionDenied diff --git a/lms/djangoapps/discussion_api/permissions.py b/lms/djangoapps/discussion_api/permissions.py new file mode 100644 index 0000000000..b76b58cbbe --- /dev/null +++ b/lms/djangoapps/discussion_api/permissions.py @@ -0,0 +1,54 @@ +""" +Discussion API permission logic +""" + + +def _is_author(cc_content, context): + """ + Return True if the requester authored the given content, False otherwise + """ + return context["cc_requester"]["id"] == cc_content["user_id"] + + +def _is_author_or_privileged(cc_content, context): + """ + Return True if the requester authored the given content or is a privileged + user, False otherwise + """ + return context["is_requester_privileged"] or _is_author(cc_content, context) + + +def get_editable_fields(cc_content, context): + """ + Return the set of fields that the requester can edit on the given content + """ + # Shared fields + ret = {"voted"} + if _is_author_or_privileged(cc_content, context): + ret |= {"raw_body"} + + # Thread fields + if cc_content["type"] == "thread": + ret |= {"following"} + if _is_author_or_privileged(cc_content, context): + ret |= {"topic_id", "type", "title"} + + # Comment fields + if ( + cc_content["type"] == "comment" and ( + context["is_requester_privileged"] or ( + _is_author(context["thread"], context) and + context["thread"]["thread_type"] == "question" + ) + ) + ): + ret |= {"endorsed"} + + return ret + + +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) diff --git a/lms/djangoapps/discussion_api/serializers.py b/lms/djangoapps/discussion_api/serializers.py index afa7309ac6..f914e4e198 100644 --- a/lms/djangoapps/discussion_api/serializers.py +++ b/lms/djangoapps/discussion_api/serializers.py @@ -10,6 +10,7 @@ from django.core.urlresolvers import reverse from rest_framework import serializers +from discussion_api.permissions import get_editable_fields from discussion_api.render import render_body from django_comment_common.models import ( FORUM_ROLE_ADMINISTRATOR, @@ -70,6 +71,7 @@ class _ContentSerializer(serializers.Serializer): abuse_flagged = serializers.SerializerMethodField("get_abuse_flagged") voted = serializers.SerializerMethodField("get_voted") vote_count = serializers.SerializerMethodField("get_vote_count") + editable_fields = serializers.SerializerMethodField("get_editable_fields") non_updatable_fields = () @@ -146,6 +148,10 @@ class _ContentSerializer(serializers.Serializer): """Returns the number of votes for the content.""" return obj["votes"]["up_count"] + def get_editable_fields(self, obj): + """Return the list of the fields the requester can edit""" + return sorted(get_editable_fields(obj, self.context)) + class ThreadSerializer(_ContentSerializer): """ diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index 4afb375b5d..e46a95e527 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/lms/djangoapps/discussion_api/tests/test_api.py @@ -541,6 +541,7 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest def test_thread_content(self): source_threads = [ { + "type": "thread", "id": "test_thread_id_0", "course_id": unicode(self.course.id), "commentable_id": "topic_x", @@ -562,6 +563,7 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest "unread_comments_count": 3, }, { + "type": "thread", "id": "test_thread_id_1", "course_id": unicode(self.course.id), "commentable_id": "topic_y", @@ -609,6 +611,7 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest "comment_list_url": "http://testserver/api/discussion/v1/comments/?thread_id=test_thread_id_0", "endorsed_comment_list_url": None, "non_endorsed_comment_list_url": None, + "editable_fields": ["following", "voted"], }, { "id": "test_thread_id_1", @@ -639,6 +642,7 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest "non_endorsed_comment_list_url": ( "http://testserver/api/discussion/v1/comments/?thread_id=test_thread_id_1&endorsed=False" ), + "editable_fields": ["following", "voted"], }, ] self.assertEqual( @@ -915,6 +919,7 @@ class GetCommentListTest(CommentsServiceMockMixin, ModuleStoreTestCase): def test_discussion_content(self): source_comments = [ { + "type": "comment", "id": "test_comment_1", "thread_id": "test_thread", "user_id": str(self.author.id), @@ -930,6 +935,7 @@ class GetCommentListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "children": [], }, { + "type": "comment", "id": "test_comment_2", "thread_id": "test_thread", "user_id": str(self.author.id), @@ -964,6 +970,7 @@ class GetCommentListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "voted": False, "vote_count": 4, "children": [], + "editable_fields": ["voted"], }, { "id": "test_comment_2", @@ -983,6 +990,7 @@ class GetCommentListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "voted": False, "vote_count": 7, "children": [], + "editable_fields": ["voted"], }, ] actual_comments = self.get_comment_list( @@ -1202,6 +1210,7 @@ class CreateThreadTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTestC "comment_list_url": "http://testserver/api/discussion/v1/comments/?thread_id=test_id", "endorsed_comment_list_url": None, "non_endorsed_comment_list_url": None, + "editable_fields": ["following", "raw_body", "title", "topic_id", "type", "voted"], } self.assertEqual(actual, expected) self.assertEqual( @@ -1367,6 +1376,7 @@ class CreateCommentTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest "voted": False, "vote_count": 0, "children": [], + "editable_fields": ["raw_body", "voted"] } self.assertEqual(actual, expected) expected_url = ( @@ -1535,7 +1545,7 @@ class UpdateThreadTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTestC "user_id": str(self.user.id), "created_at": "2015-05-29T00:00:00Z", "updated_at": "2015-05-29T00:00:00Z", - "type": "discussion", + "thread_type": "discussion", "title": "Original Title", "body": "Original body", }) @@ -1580,6 +1590,7 @@ class UpdateThreadTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTestC "comment_list_url": "http://testserver/api/discussion/v1/comments/?thread_id=test_thread", "endorsed_comment_list_url": None, "non_endorsed_comment_list_url": None, + "editable_fields": ["following", "raw_body", "title", "topic_id", "type", "voted"], } self.assertEqual(actual, expected) self.assertEqual( @@ -1841,6 +1852,7 @@ class UpdateCommentTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest "voted": False, "vote_count": 0, "children": [], + "editable_fields": ["raw_body", "voted"] } self.assertEqual(actual, expected) self.assertEqual( @@ -1959,19 +1971,24 @@ class UpdateCommentTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest FORUM_ROLE_STUDENT, ], [True, False], + ["question", "discussion"], [True, False], )) @ddt.unpack - def test_endorsed_access(self, role_name, is_thread_author, is_comment_author): + def test_endorsed_access(self, role_name, is_thread_author, thread_type, is_comment_author): role = Role.objects.create(name=role_name, course_id=self.course.id) role.users = [self.user] self.register_comment( {"user_id": str(self.user.id if is_comment_author else (self.user.id + 1))}, thread_overrides={ - "user_id": str(self.user.id if is_thread_author else (self.user.id + 1)) + "thread_type": thread_type, + "user_id": str(self.user.id if is_thread_author else (self.user.id + 1)), } ) - expected_error = role_name == FORUM_ROLE_STUDENT and not is_thread_author + expected_error = ( + role_name == FORUM_ROLE_STUDENT and + (thread_type == "discussion" or not is_thread_author) + ) try: update_comment(self.request, "test_comment", {"endorsed": True}) self.assertFalse(expected_error) diff --git a/lms/djangoapps/discussion_api/tests/test_permissions.py b/lms/djangoapps/discussion_api/tests/test_permissions.py new file mode 100644 index 0000000000..66e8e84a8d --- /dev/null +++ b/lms/djangoapps/discussion_api/tests/test_permissions.py @@ -0,0 +1,75 @@ +""" +Tests for discussion API permission logic +""" +import itertools +from unittest import TestCase + +import ddt + +from discussion_api.permissions import can_delete, get_editable_fields +from lms.lib.comment_client.comment import Comment +from lms.lib.comment_client.thread import Thread +from lms.lib.comment_client.user import User + + +def _get_context(requester_id, is_requester_privileged, thread=None): + """Return a context suitable for testing the permissions module""" + return { + "cc_requester": User(id=requester_id), + "is_requester_privileged": is_requester_privileged, + "thread": thread, + } + + +@ddt.ddt +class GetEditableFieldsTest(TestCase): + """Tests for get_editable_fields""" + @ddt.data(*itertools.product([True, False], [True, False])) + @ddt.unpack + def test_thread(self, is_author, is_privileged): + thread = Thread(user_id="5" if is_author else "6", type="thread") + context = _get_context(requester_id="5", is_requester_privileged=is_privileged) + actual = get_editable_fields(thread, context) + expected = {"following", "voted"} + if is_author or is_privileged: + expected |= {"topic_id", "type", "title", "raw_body"} + self.assertEqual(actual, expected) + + @ddt.data(*itertools.product([True, False], [True, False], ["question", "discussion"], [True, False])) + @ddt.unpack + def test_comment(self, is_author, is_thread_author, thread_type, is_privileged): + comment = Comment(user_id="5" if is_author else "6", type="comment") + context = _get_context( + requester_id="5", + is_requester_privileged=is_privileged, + thread=Thread(user_id="5" if is_thread_author else "6", thread_type=thread_type) + ) + actual = get_editable_fields(comment, context) + expected = {"voted"} + if is_author or is_privileged: + expected |= {"raw_body"} + if (is_thread_author and thread_type == "question") or is_privileged: + expected |= {"endorsed"} + self.assertEqual(actual, expected) + + +@ddt.ddt +class CanDeleteTest(TestCase): + """Tests for can_delete""" + @ddt.data(*itertools.product([True, False], [True, False])) + @ddt.unpack + def test_thread(self, is_author, is_privileged): + thread = Thread(user_id="5" if is_author else "6") + context = _get_context(requester_id="5", is_requester_privileged=is_privileged) + self.assertEqual(can_delete(thread, context), is_author or is_privileged) + + @ddt.data(*itertools.product([True, False], [True, False], [True, False])) + @ddt.unpack + def test_comment(self, is_author, is_thread_author, is_privileged): + comment = Comment(user_id="5" if is_author else "6") + context = _get_context( + requester_id="5", + is_requester_privileged=is_privileged, + thread=Thread(user_id="5" if is_thread_author else "6") + ) + self.assertEqual(can_delete(comment, context), is_author or is_privileged) diff --git a/lms/djangoapps/discussion_api/tests/test_serializers.py b/lms/djangoapps/discussion_api/tests/test_serializers.py index a1b97a110d..6ee19b657e 100644 --- a/lms/djangoapps/discussion_api/tests/test_serializers.py +++ b/lms/djangoapps/discussion_api/tests/test_serializers.py @@ -151,6 +151,7 @@ class ThreadSerializerSerializationTest(SerializerTestMixin, ModuleStoreTestCase def test_basic(self): thread = { + "type": "thread", "id": "test_thread", "course_id": unicode(self.course.id), "commentable_id": "test_topic", @@ -196,6 +197,7 @@ class ThreadSerializerSerializationTest(SerializerTestMixin, ModuleStoreTestCase "comment_list_url": "http://testserver/api/discussion/v1/comments/?thread_id=test_thread", "endorsed_comment_list_url": None, "non_endorsed_comment_list_url": None, + "editable_fields": ["following", "voted"], } self.assertEqual(self.serialize(thread), expected) @@ -270,6 +272,7 @@ class CommentSerializerTest(SerializerTestMixin, ModuleStoreTestCase): def test_basic(self): comment = { + "type": "comment", "id": "test_comment", "thread_id": "test_thread", "user_id": str(self.author.id), @@ -302,6 +305,7 @@ class CommentSerializerTest(SerializerTestMixin, ModuleStoreTestCase): "voted": False, "vote_count": 4, "children": [], + "editable_fields": ["voted"], } self.assertEqual(self.serialize(comment), expected) diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index 8bdaa6f2a6..302badbc14 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -158,6 +158,7 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): def test_basic(self): self.register_get_user_response(self.user, upvoted_ids=["test_thread"]) source_threads = [{ + "type": "thread", "id": "test_thread", "course_id": unicode(self.course.id), "commentable_id": "test_topic", @@ -203,6 +204,7 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "comment_list_url": "http://testserver/api/discussion/v1/comments/?thread_id=test_thread", "endorsed_comment_list_url": None, "non_endorsed_comment_list_url": None, + "editable_fields": ["following", "voted"], }] self.register_get_threads_response(source_threads, page=1, num_pages=2) response = self.client.get(self.url, {"course_id": unicode(self.course.id)}) @@ -316,6 +318,7 @@ class ThreadViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "comment_list_url": "http://testserver/api/discussion/v1/comments/?thread_id=test_thread", "endorsed_comment_list_url": None, "non_endorsed_comment_list_url": None, + "editable_fields": ["following", "raw_body", "title", "topic_id", "type", "voted"], } response = self.client.post( self.url, @@ -406,6 +409,7 @@ class ThreadViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTest "comment_list_url": "http://testserver/api/discussion/v1/comments/?thread_id=test_thread", "endorsed_comment_list_url": None, "non_endorsed_comment_list_url": None, + "editable_fields": ["following", "raw_body", "title", "topic_id", "type", "voted"], } response = self.client.patch( # pylint: disable=no-member self.url, @@ -515,6 +519,7 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): def test_basic(self): self.register_get_user_response(self.user, upvoted_ids=["test_comment"]) source_comments = [{ + "type": "comment", "id": "test_comment", "thread_id": self.thread_id, "parent_id": None, @@ -548,6 +553,7 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "voted": True, "vote_count": 4, "children": [], + "editable_fields": ["voted"], }] self.register_get_thread_response({ "id": self.thread_id, @@ -701,6 +707,7 @@ class CommentViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "voted": False, "vote_count": 0, "children": [], + "editable_fields": ["raw_body", "voted"], } response = self.client.post( self.url, @@ -784,6 +791,7 @@ class CommentViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTes "voted": False, "vote_count": 0, "children": [], + "editable_fields": ["raw_body", "voted"], } response = self.client.patch( # pylint: disable=no-member self.url, diff --git a/lms/djangoapps/discussion_api/tests/utils.py b/lms/djangoapps/discussion_api/tests/utils.py index cd7b6a9f97..5dc0e3f5f5 100644 --- a/lms/djangoapps/discussion_api/tests/utils.py +++ b/lms/djangoapps/discussion_api/tests/utils.py @@ -273,6 +273,7 @@ def make_minimal_cs_thread(overrides=None): comments service with dummy data and optional overrides """ ret = { + "type": "thread", "id": "dummy", "course_id": "dummy/dummy/dummy", "commentable_id": "dummy", @@ -305,6 +306,7 @@ def make_minimal_cs_comment(overrides=None): comments service with dummy data and optional overrides """ ret = { + "type": "comment", "id": "dummy", "thread_id": "dummy", "parent_id": None, diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index d5d6c41b37..0658bcf382 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -203,6 +203,9 @@ class ThreadViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): that were created or updated since the last time the user read the thread + * editable_fields: The fields that the requesting user is allowed to + modify with a PATCH request + **DELETE response values: No content is returned for a DELETE request @@ -352,6 +355,9 @@ class CommentViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): * children: The list of child comments (with the same format) + * editable_fields: The fields that the requesting user is allowed to + modify with a PATCH request + **DELETE Response Value** No content is returned for a DELETE request