From 01c22531d7f5ec9a0b6aa4c7c8cdc6857b60a028 Mon Sep 17 00:00:00 2001 From: cahrens Date: Wed, 22 Jul 2015 17:24:07 -0400 Subject: [PATCH] Add team permission check on commentable_id. TNL-2864 --- .../django_comment_client/base/tests.py | 246 +++++++++++++++++- .../django_comment_client/base/views.py | 26 +- .../django_comment_client/forum/tests.py | 8 +- .../django_comment_client/permissions.py | 62 +++-- lms/lib/comment_client/commentable.py | 10 + 5 files changed, 293 insertions(+), 59 deletions(-) diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index 07458a17c5..dbcab2bd00 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/lms/djangoapps/django_comment_client/base/tests.py @@ -28,6 +28,8 @@ from xmodule.modulestore.tests.factories import check_mongo_calls from xmodule.modulestore.django import modulestore from xmodule.modulestore import ModuleStoreEnum +from teams.tests.factories import CourseTeamFactory + log = logging.getLogger(__name__) @@ -99,7 +101,8 @@ class ThreadActionGroupIdTestCase( "user_id": str(self.student.id), "group_id": self.student_cohort.id, "closed": False, - "type": "thread" + "type": "thread", + "commentable_id": "non_team_dummy_id" } ) mock_request.return_value.status_code = 200 @@ -231,6 +234,7 @@ class ViewsTestCaseMixin(object): data = { "user_id": str(self.student.id), "closed": False, + "commentable_id": "non_team_dummy_id" } if include_depth: data["depth"] = 0 @@ -347,10 +351,10 @@ class ViewsQueryCountTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSet return inner @ddt.data( - (ModuleStoreEnum.Type.mongo, 3, 4, 21), - (ModuleStoreEnum.Type.mongo, 20, 4, 21), - (ModuleStoreEnum.Type.split, 3, 13, 21), - (ModuleStoreEnum.Type.split, 20, 13, 21), + (ModuleStoreEnum.Type.mongo, 3, 4, 22), + (ModuleStoreEnum.Type.mongo, 20, 4, 22), + (ModuleStoreEnum.Type.split, 3, 13, 22), + (ModuleStoreEnum.Type.split, 20, 13, 22), ) @ddt.unpack @count_queries @@ -964,7 +968,9 @@ class CreateThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockReq request = RequestFactory().post("dummy_url", {"thread_type": "discussion", "body": text, "title": text}) request.user = self.student request.view_name = "create_thread" - response = views.create_thread(request, course_id=self.course.id.to_deprecated_string(), commentable_id="test_commentable") + response = views.create_thread( + request, course_id=self.course.id.to_deprecated_string(), commentable_id="non_team_dummy_id" + ) self.assertEqual(response.status_code, 200) self.assertTrue(mock_request.called) @@ -1012,13 +1018,15 @@ class CreateCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, MockRe @patch('lms.lib.comment_client.utils.requests.request') def _test_unicode_data(self, text, mock_request): + commentable_id = "non_team_dummy_id" self._set_mock_request_data(mock_request, { "closed": False, + "commentable_id": commentable_id }) # We have to get clever here due to Thread's setters and getters. # Patch won't work with it. try: - Thread.commentable_id = Mock() + Thread.commentable_id = commentable_id request = RequestFactory().post("dummy_url", {"body": text}) request.user = self.student request.view_name = "create_comment" @@ -1078,7 +1086,8 @@ class CreateSubCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, Moc self._set_mock_request_data(mock_request, { "closed": False, "depth": 1, - "thread_id": "test_thread" + "thread_id": "test_thread", + "commentable_id": "non_team_dummy_id" }) request = RequestFactory().post("dummy_url", {"body": text}) request.user = self.student @@ -1086,7 +1095,7 @@ class CreateSubCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, Moc Thread.commentable_id = Mock() try: response = views.create_sub_comment( - request, course_id=self.course.id.to_deprecated_string(), comment_id="dummy_comment_id" + request, course_id=unicode(self.course.id), comment_id="dummy_comment_id" ) self.assertEqual(response.status_code, 200) @@ -1096,6 +1105,219 @@ class CreateSubCommentUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin, Moc del Thread.commentable_id +@ddt.ddt +@patch("lms.lib.comment_client.utils.requests.request") +class TeamsPermissionsTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin): + # Most of the test points use the same ddt data. + # args: user, commentable_id, status_code + ddt_permissions_args = [ + # Student in team can do operations on threads/comments within the team commentable. + ('student_in_team', 'team_commentable_id', 200), + # Non-team commentables can be edited by any student. + ('student_in_team', 'course_commentable_id', 200), + # Student not in team cannot do operations within the team commentable. + ('student_not_in_team', 'team_commentable_id', 401), + # Non-team commentables can be edited by any student. + ('student_not_in_team', 'course_commentable_id', 200), + # Moderators most be a member of the team for doing "student actions". + ('moderator', 'team_commentable_id', 401) + ] + + @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) + def setUp(self): + super(TeamsPermissionsTestCase, self).setUp() + self.password = "test password" + teams_configuration = { + 'topics': [{'id': "topic_id", 'name': 'Solar Power', 'description': 'Solar power is hot'}] + } + self.course = CourseFactory.create(teams_configuration=teams_configuration) + seed_permissions_roles(self.course.id) + + # Create 3 users-- student in team, student not in team, discussion moderator + self.student_in_team = UserFactory.create(password=self.password) + self.student_not_in_team = UserFactory.create(password=self.password) + self.moderator = UserFactory.create(password=self.password) + CourseEnrollmentFactory(user=self.student_in_team, course_id=self.course.id) + CourseEnrollmentFactory(user=self.student_not_in_team, course_id=self.course.id) + CourseEnrollmentFactory(user=self.moderator, course_id=self.course.id) + self.moderator.roles.add(Role.objects.get(name="Moderator", course_id=self.course.id)) + + # Create a team. + self.team_commentable_id = "team_discussion_id" + self.team = CourseTeamFactory.create( + name=u'The Only Team', + course_id=self.course.id, + topic_id='topic_id', + discussion_topic_id=self.team_commentable_id + ) + self.team.add_user(self.student_in_team) + + # Dummy commentable ID not linked to a team + self.course_commentable_id = "course_level_commentable" + + def _setup_mock(self, user, mock_request, data): + user = getattr(self, user) + self._set_mock_request_data(mock_request, data) + self.client.login(username=user.username, password=self.password) + + @ddt.data( + # student_in_team will be able to update his own post, regardless of team membership + ('student_in_team', 'student_in_team', 'team_commentable_id', 200), + ('student_in_team', 'student_in_team', 'course_commentable_id', 200), + # students can only update their own posts + ('student_in_team', 'moderator', 'team_commentable_id', 401), + # Even though student_not_in_team is not in the team, he can still modify posts he created while in the team. + ('student_not_in_team', 'student_not_in_team', 'team_commentable_id', 200), + # Moderators can change their own posts and other people's posts. + ('moderator', 'moderator', 'team_commentable_id', 200), + ('moderator', 'student_in_team', 'team_commentable_id', 200), + ) + @ddt.unpack + def test_update_thread(self, user, thread_author, commentable_id, status_code, mock_request): + """ + Verify that update_thread is limited to thread authors and privileged users (team membership does not matter). + """ + commentable_id = getattr(self, commentable_id) + # thread_author is who is marked as the author of the thread being updated. + thread_author = getattr(self, thread_author) + self._setup_mock( + user, mock_request, # user is the person making the request. + {"user_id": str(thread_author.id), "closed": False, "commentable_id": commentable_id} + ) + response = self.client.post( + reverse( + "update_thread", + kwargs={ + "course_id": unicode(self.course.id), + "thread_id": "dummy" + } + ), + data={"body": "foo", "title": "foo"} + ) + self.assertEqual(response.status_code, status_code) + + @ddt.data(*ddt_permissions_args) + @ddt.unpack + def test_create_comment(self, user, commentable_id, status_code, mock_request): + """ + Verify that create_comment is limited to members of the team. + """ + commentable_id = getattr(self, commentable_id) + self._setup_mock(user, mock_request, {"closed": False, "commentable_id": commentable_id}) + + response = self.client.post( + reverse( + "create_comment", + kwargs={ + "course_id": unicode(self.course.id), + "thread_id": "dummy" + } + ), + data={"body": "foo", "title": "foo"} + ) + self.assertEqual(response.status_code, status_code) + + @ddt.data(*ddt_permissions_args) + @ddt.unpack + def test_create_sub_comment(self, user, commentable_id, status_code, mock_request): + """ + Verify that create_subcomment is limited to members of the team. + """ + commentable_id = getattr(self, commentable_id) + self._setup_mock( + user, mock_request, + {"closed": False, "commentable_id": commentable_id, "thread_id": "dummy_thread"}, + ) + response = self.client.post( + reverse( + "create_sub_comment", + kwargs={ + "course_id": unicode(self.course.id), + "comment_id": "dummy_comment" + } + ), + data={"body": "foo", "title": "foo"} + ) + self.assertEqual(response.status_code, status_code) + + @ddt.data(*ddt_permissions_args) + @ddt.unpack + def test_comment_actions(self, user, commentable_id, status_code, mock_request): + """ + Verify that voting and flagging of comments is limited to members of the team. + """ + commentable_id = getattr(self, commentable_id) + self._setup_mock( + user, mock_request, + {"closed": False, "commentable_id": commentable_id, "thread_id": "dummy_thread"}, + ) + for action in ["upvote_comment", "downvote_comment", "un_flag_abuse_for_comment", "flag_abuse_for_comment"]: + response = self.client.post( + reverse( + action, + kwargs={"course_id": unicode(self.course.id), "comment_id": "dummy_comment"} + ) + ) + self.assertEqual(response.status_code, status_code) + + @ddt.data(*ddt_permissions_args) + @ddt.unpack + def test_threads_actions(self, user, commentable_id, status_code, mock_request): + """ + Verify that voting, flagging, and following of threads is limited to members of the team. + """ + commentable_id = getattr(self, commentable_id) + self._setup_mock( + user, mock_request, + {"closed": False, "commentable_id": commentable_id}, + ) + for action in ["upvote_thread", "downvote_thread", "un_flag_abuse_for_thread", "flag_abuse_for_thread", + "follow_thread", "unfollow_thread"]: + response = self.client.post( + reverse( + action, + kwargs={"course_id": unicode(self.course.id), "thread_id": "dummy_thread"} + ) + ) + self.assertEqual(response.status_code, status_code) + + @ddt.data(*ddt_permissions_args) + @ddt.unpack + def test_create_thread(self, user, commentable_id, status_code, __): + """ + Verify that creation of threads is limited to members of the team. + """ + commentable_id = getattr(self, commentable_id) + # mock_request is not used because Commentables don't exist in comment service. + self.client.login(username=getattr(self, user).username, password=self.password) + response = self.client.post( + reverse( + "create_thread", + kwargs={"course_id": unicode(self.course.id), "commentable_id": commentable_id} + ), + data={"body": "foo", "title": "foo", "thread_type": "discussion"} + ) + self.assertEqual(response.status_code, status_code) + + @ddt.data(*ddt_permissions_args) + @ddt.unpack + def test_commentable_actions(self, user, commentable_id, status_code, __): + """ + Verify that following of commentables is limited to members of the team. + """ + commentable_id = getattr(self, commentable_id) + # mock_request is not used because Commentables don't exist in comment service. + self.client.login(username=getattr(self, user).username, password=self.password) + for action in ["follow_commentable", "unfollow_commentable"]: + response = self.client.post( + reverse( + action, + kwargs={"course_id": unicode(self.course.id), "commentable_id": commentable_id} + ) + ) + self.assertEqual(response.status_code, status_code) + + class ForumEventTestCase(ModuleStoreTestCase, MockRequestSetupMixin): """ Forum actions are expected to launch analytics events. Test these here. @@ -1123,7 +1345,7 @@ class ForumEventTestCase(ModuleStoreTestCase, MockRequestSetupMixin): request.user = self.student request.view_name = "create_thread" - views.create_thread(request, course_id=self.course.id.to_deprecated_string(), commentable_id="test_commentable") + views.create_thread(request, course_id=unicode(self.course.id), commentable_id="test_commentable") event_name, event = mock_emit.call_args[0] self.assertEqual(event_name, 'edx.forum.thread.created') @@ -1180,9 +1402,7 @@ class ForumEventTestCase(ModuleStoreTestCase, MockRequestSetupMixin): request = RequestFactory().post("dummy_url", {"body": "Another comment"}) request.user = self.student request.view_name = "create_sub_comment" - views.create_sub_comment( - request, course_id=self.course.id.to_deprecated_string(), comment_id="dummy_comment_id" - ) + views.create_sub_comment(request, course_id=unicode(self.course.id), comment_id="dummy_comment_id") event_name, event = mock_emit.call_args[0] self.assertEqual(event_name, "edx.forum.comment.created") diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index 5456e970db..0111927ce6 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/lms/djangoapps/django_comment_client/base/views.py @@ -51,6 +51,8 @@ def permitted(fn): content = cc.Thread.find(kwargs["thread_id"]).to_dict() elif "comment_id" in kwargs: content = cc.Comment.find(kwargs["comment_id"]).to_dict() + elif "commentable_id" in kwargs: + content = cc.Commentable.find(kwargs["commentable_id"]).to_dict() else: content = None return content @@ -605,16 +607,6 @@ def follow_commentable(request, course_id, commentable_id): return JsonResponse({}) -@require_POST -@login_required -@permitted -def follow_user(request, course_id, followed_user_id): - user = cc.User.from_django_user(request.user) - followed_user = cc.User.find(followed_user_id) - user.follow(followed_user) - return JsonResponse({}) - - @require_POST @login_required @permitted @@ -643,20 +635,6 @@ def unfollow_commentable(request, course_id, commentable_id): return JsonResponse({}) -@require_POST -@login_required -@permitted -def unfollow_user(request, course_id, followed_user_id): - """ - given a course id and user id, stop following this user - ajax only - """ - user = cc.User.from_django_user(request.user) - followed_user = cc.User.find(followed_user_id) - user.unfollow(followed_user) - return JsonResponse({}) - - @require_POST @login_required @csrf.csrf_exempt diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index e542c4ba39..4d076a5705 100644 --- a/lms/djangoapps/django_comment_client/forum/tests.py +++ b/lms/djangoapps/django_comment_client/forum/tests.py @@ -333,11 +333,11 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase): @ddt.data( # old mongo with cache - (ModuleStoreEnum.Type.mongo, 1, 7, 5, 13, 8), - (ModuleStoreEnum.Type.mongo, 50, 7, 5, 13, 8), + (ModuleStoreEnum.Type.mongo, 1, 7, 5, 14, 8), + (ModuleStoreEnum.Type.mongo, 50, 7, 5, 14, 8), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, 1, 3, 3, 13, 8), - (ModuleStoreEnum.Type.split, 50, 3, 3, 13, 8), + (ModuleStoreEnum.Type.split, 1, 3, 3, 14, 8), + (ModuleStoreEnum.Type.split, 50, 3, 3, 14, 8), ) @ddt.unpack def test_number_of_mongo_queries( diff --git a/lms/djangoapps/django_comment_client/permissions.py b/lms/djangoapps/django_comment_client/permissions.py index c7687fbe74..f0b72e20fe 100644 --- a/lms/djangoapps/django_comment_client/permissions.py +++ b/lms/djangoapps/django_comment_client/permissions.py @@ -10,6 +10,7 @@ from lms.lib.comment_client import Thread from opaque_keys.edx.keys import CourseKey from django_comment_common.models import all_permissions_for_user_in_course +from teams.models import CourseTeam def has_permission(user, permission, course_id=None): @@ -27,7 +28,7 @@ def has_permission(user, permission, course_id=None): return permission in all_permissions -CONDITIONS = ['is_open', 'is_author', 'is_question_author'] +CONDITIONS = ['is_open', 'is_author', 'is_question_author', 'is_team_member_if_applicable'] def _check_condition(user, condition, content): @@ -55,10 +56,37 @@ def _check_condition(user, condition, content): except KeyError: return False + def check_team_member(user, content): + """ + If the content has a commentable_id, verifies that either it is not associated with a team, + or if it is, that the user is a member of that team. + """ + if not content: + return False + try: + commentable_id = content['commentable_id'] + request_cache_dict = RequestCache.get_request_cache().data + cache_key = "django_comment_client.check_team_member.{}.{}".format(user.id, commentable_id) + if cache_key in request_cache_dict: + return request_cache_dict[cache_key] + team = CourseTeam.objects.get(discussion_topic_id=commentable_id) + passes_condition = team.users.filter(id=user.id).count() > 0 + request_cache_dict[cache_key] = passes_condition + except KeyError: + # We do not expect KeyError in production-- it usually indicates an improper test mock. + logging.warning("Did not find key commentable_id in content.") + passes_condition = False + except CourseTeam.DoesNotExist: + passes_condition = True + request_cache_dict[cache_key] = passes_condition + + return passes_condition + handlers = { 'is_open': check_open, 'is_author': check_author, 'is_question_author': check_question_author, + 'is_team_member_if_applicable': check_team_member } return handlers[condition](user, content) @@ -88,30 +116,28 @@ def _check_conditions_permissions(user, permissions, course_id, content): VIEW_PERMISSIONS = { 'update_thread': ['edit_content', ['update_thread', 'is_open', 'is_author']], - 'create_comment': [["create_comment", "is_open"]], + 'create_comment': [["create_comment", "is_open", "is_team_member_if_applicable"]], 'delete_thread': ['delete_thread', ['update_thread', 'is_author']], 'update_comment': ['edit_content', ['update_comment', 'is_open', 'is_author']], 'endorse_comment': ['endorse_comment', 'is_question_author'], 'openclose_thread': ['openclose_thread'], - 'create_sub_comment': [['create_sub_comment', 'is_open']], + 'create_sub_comment': [['create_sub_comment', 'is_open', 'is_team_member_if_applicable']], 'delete_comment': ['delete_comment', ['update_comment', 'is_open', 'is_author']], - 'vote_for_comment': [['vote', 'is_open']], - 'undo_vote_for_comment': [['unvote', 'is_open']], - 'vote_for_thread': [['vote', 'is_open']], - 'flag_abuse_for_thread': ['vote'], - 'un_flag_abuse_for_thread': ['vote'], - 'flag_abuse_for_comment': ['vote'], - 'un_flag_abuse_for_comment': ['vote'], - 'undo_vote_for_thread': [['unvote', 'is_open']], + 'vote_for_comment': [['vote', 'is_open', 'is_team_member_if_applicable']], + 'undo_vote_for_comment': [['unvote', 'is_open', 'is_team_member_if_applicable']], + 'vote_for_thread': [['vote', 'is_open', 'is_team_member_if_applicable']], + 'flag_abuse_for_thread': [['vote', 'is_team_member_if_applicable']], + 'un_flag_abuse_for_thread': [['vote', 'is_team_member_if_applicable']], + 'flag_abuse_for_comment': [['vote', 'is_team_member_if_applicable']], + 'un_flag_abuse_for_comment': [['vote', 'is_team_member_if_applicable']], + 'undo_vote_for_thread': [['unvote', 'is_open', 'is_team_member_if_applicable']], 'pin_thread': ['openclose_thread'], 'un_pin_thread': ['openclose_thread'], - 'follow_thread': ['follow_thread'], - 'follow_commentable': ['follow_commentable'], - 'follow_user': ['follow_user'], - 'unfollow_thread': ['unfollow_thread'], - 'unfollow_commentable': ['unfollow_commentable'], - 'unfollow_user': ['unfollow_user'], - 'create_thread': ['create_thread'], + 'follow_thread': [['follow_thread', 'is_team_member_if_applicable']], + 'follow_commentable': [['follow_commentable', 'is_team_member_if_applicable']], + 'unfollow_thread': [['unfollow_thread', 'is_team_member_if_applicable']], + 'unfollow_commentable': [['unfollow_commentable', 'is_team_member_if_applicable']], + 'create_thread': [['create_thread', 'is_team_member_if_applicable']], } diff --git a/lms/lib/comment_client/commentable.py b/lms/lib/comment_client/commentable.py index 9ce3b20053..93a2c28022 100644 --- a/lms/lib/comment_client/commentable.py +++ b/lms/lib/comment_client/commentable.py @@ -5,5 +5,15 @@ from lms.lib.comment_client import settings class Commentable(models.Model): + accessible_fields = ['id', 'commentable_id'] + base_url = "{prefix}/commentables".format(prefix=settings.PREFIX) type = 'commentable' + + def retrieve(self, *args, **kwargs): + """ + Override default behavior because commentables don't actually exist in the comment service. + """ + self.attributes["commentable_id"] = self.attributes["id"] + self.retrieved = True + return self