From 350d15876ab1d813ac41e01c7a85f3b2a3df1d36 Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Fri, 14 Aug 2015 14:21:50 -0400 Subject: [PATCH 1/2] Expand instructor definition Per discussions, for the purposes of the teams API, an 'instructor' is any of: -course staff -global staff -discussion privileged users This change will include the last case, which previously did not have instructor access. Changes will be documented on the teams API wiki: https://openedx.atlassian.net/wiki/display/TNL/Team+API Tests have also been added to confirm this functionality. TNL-2984 --- lms/djangoapps/teams/tests/test_views.py | 14 +++++- lms/djangoapps/teams/views.py | 59 +++++++++++++++--------- 2 files changed, 51 insertions(+), 22 deletions(-) diff --git a/lms/djangoapps/teams/tests/test_views.py b/lms/djangoapps/teams/tests/test_views.py index b97252fadd..70efdbcc4d 100644 --- a/lms/djangoapps/teams/tests/test_views.py +++ b/lms/djangoapps/teams/tests/test_views.py @@ -375,6 +375,7 @@ class TestListTeamsAPI(TeamAPITestCase): ('student_enrolled', 200), ('staff', 200), ('course_staff', 200), + ('community_ta', 200), ) @ddt.unpack def test_access(self, user, status): @@ -467,7 +468,8 @@ class TestCreateTeamAPI(TeamAPITestCase): ('student_unenrolled', 403), ('student_enrolled_not_on_team', 200), ('staff', 200), - ('course_staff', 200) + ('course_staff', 200), + ('community_ta', 200), ) @ddt.unpack def test_access(self, user, status): @@ -580,6 +582,7 @@ class TestDetailTeamAPI(TeamAPITestCase): ('student_enrolled', 200), ('staff', 200), ('course_staff', 200), + ('community_ta', 200), ) @ddt.unpack def test_access(self, user, status): @@ -617,6 +620,7 @@ class TestUpdateTeamAPI(TeamAPITestCase): ('student_enrolled', 403), ('staff', 200), ('course_staff', 200), + ('community_ta', 200), ) @ddt.unpack def test_access(self, user, status): @@ -631,6 +635,7 @@ class TestUpdateTeamAPI(TeamAPITestCase): ('student_enrolled', 404), ('staff', 404), ('course_staff', 404), + ('community_ta', 404), ) @ddt.unpack def test_access_bad_id(self, user, status): @@ -666,6 +671,7 @@ class TestListTopicsAPI(TeamAPITestCase): ('student_enrolled', 200), ('staff', 200), ('course_staff', 200), + ('community_ta', 200), ) @ddt.unpack def test_access(self, user, status): @@ -733,6 +739,7 @@ class TestDetailTopicAPI(TeamAPITestCase): ('student_enrolled', 200), ('staff', 200), ('course_staff', 200), + ('community_ta', 200), ) @ddt.unpack def test_access(self, user, status): @@ -768,6 +775,7 @@ class TestListMembershipAPI(TeamAPITestCase): ('student_enrolled_both_courses_other_team', 200), ('staff', 200), ('course_staff', 200), + ('community_ta', 200), ) @ddt.unpack def test_access(self, user, status): @@ -784,6 +792,7 @@ class TestListMembershipAPI(TeamAPITestCase): ('student_enrolled_both_courses_other_team', 200, True), ('staff', 200, True), ('course_staff', 200, True), + ('community_ta', 200, True), ) @ddt.unpack def test_access_by_username(self, user, status, has_content): @@ -874,6 +883,7 @@ class TestCreateMembershipAPI(TeamAPITestCase): ('student_enrolled_both_courses_other_team', 404), ('staff', 200), ('course_staff', 200), + ('community_ta', 200), ) @ddt.unpack def test_access(self, user, status): @@ -948,6 +958,7 @@ class TestDetailMembershipAPI(TeamAPITestCase): ('student_enrolled', 200), ('staff', 200), ('course_staff', 200), + ('community_ta', 200), ) @ddt.unpack def test_access(self, user, status): @@ -1013,6 +1024,7 @@ class TestDeleteMembershipAPI(TeamAPITestCase): ('student_enrolled', 204), ('staff', 204), ('course_staff', 204), + ('community_ta', 204), ) @ddt.unpack def test_access(self, user, status): diff --git a/lms/djangoapps/teams/views.py b/lms/djangoapps/teams/views.py index 91038b3145..2605a70322 100644 --- a/lms/djangoapps/teams/views.py +++ b/lms/djangoapps/teams/views.py @@ -51,7 +51,6 @@ from .serializers import ( ) from .errors import AlreadyOnTeamInCourse, NotEnrolledInCourseForTeam - TEAM_MEMBERSHIPS_PER_PAGE = 2 TOPICS_PER_PAGE = 12 @@ -120,7 +119,7 @@ class TeamsDashboardView(View): def has_team_api_access(user, course_key, access_username=None): """Returns True if the user has access to the Team API for the course given by `course_key`. The user must either be enrolled in the course, - be course staff, or be global staff. + be course staff, be global staff, or have discussion privileges. Args: user (User): The user to check access for. @@ -134,6 +133,8 @@ def has_team_api_access(user, course_key, access_username=None): return True if CourseStaffRole(course_key).has_user(user): return True + if has_discussion_privileges(user, course_key): + return True if not access_username or access_username == user.username: return CourseEnrollment.is_enrolled(user, course_key) return False @@ -250,8 +251,9 @@ class TeamsListView(ExpandableFieldViewMixin, GenericAPIView): If the user is not logged in, a 401 error is returned. - If the user is not enrolled in the course, or is not course or - global staff, a 403 error is returned. + If the user is not enrolled in the course, is not course or + global staff, or does not have discussion privileges a 403 error + is returned. If the course_id is not valid or extra fields are included in the request, a 400 error is returned. @@ -467,8 +469,8 @@ class TeamsDetailView(ExpandableFieldViewMixin, RetrievePatchAPIView): If the user is anonymous or inactive, a 401 is returned. If the user is logged in and the team does not exist, a 404 is returned. - If the user is not course or global staff and the team does exist, - a 403 is returned. + If the user is not course or global staff, does not have discussion + privileges, and the team does exist, a 403 is returned. If "application/merge-patch+json" is not the specified content type, a 415 error is returned. @@ -485,8 +487,20 @@ class TeamsDetailView(ExpandableFieldViewMixin, RetrievePatchAPIView): """Returns true if the user is enrolled or is staff.""" return has_team_api_access(request.user, obj.course_id) + class IsStaffOrPrivilegedOrReadOnly(IsStaffOrReadOnly): + """Permission that checks to see if the user is global staff, course + staff, or has discussion privileges. If none of those conditions are + met, only read access will be granted. + """ + + def has_object_permission(self, request, view, obj): + return ( + has_discussion_privileges(request.user, obj.course_id) or + IsStaffOrReadOnly.has_object_permission(self, request, view, obj) + ) + authentication_classes = (OAuth2Authentication, SessionAuthentication) - permission_classes = (permissions.IsAuthenticated, IsStaffOrReadOnly, IsEnrolledOrIsStaff,) + permission_classes = (permissions.IsAuthenticated, IsStaffOrPrivilegedOrReadOnly, IsEnrolledOrIsStaff,) lookup_field = 'team_id' serializer_class = CourseTeamSerializer parser_classes = (MergePatchParser,) @@ -765,8 +779,9 @@ class MembershipListView(ExpandableFieldViewMixin, GenericAPIView): **Response Values for POST** Any logged in user enrolled in a course can enroll themselves in a - team in the course. Course and global staff can enroll any user in - a team, with a few exceptions noted below. + team in the course. Course staff, global staff, and discussion + privileged users can enroll any user in a team, with a few + exceptions noted below. If the user is not logged in and active, a 401 error is returned. @@ -775,11 +790,11 @@ class MembershipListView(ExpandableFieldViewMixin, GenericAPIView): If the specified team does not exist, a 404 error is returned. - If the user is not staff and is not enrolled in the course - associated with the team they are trying to join, or if they are - trying to add a user other than themselves to a team, a 404 error - is returned. This is to prevent leaking information about the - existence of teams and users. + If the user is not staff, does not have discussion privileges, + and is not enrolled in the course associated with the team they + are trying to join, or if they are trying to add a user other + than themselves to a team, a 404 error is returned. This is to + prevent leaking information about the existence of teams and users. If the specified user does not exist, a 404 error is returned. @@ -789,7 +804,8 @@ class MembershipListView(ExpandableFieldViewMixin, GenericAPIView): If the user is not enrolled in the course associated with the team they are trying to join, a 400 error is returned. This can occur - when a staff user posts a request adding another user to a team. + when a staff or discussion privileged user posts a request adding + another user to a team. """ authentication_classes = (OAuth2Authentication, SessionAuthentication) @@ -961,18 +977,19 @@ class MembershipDetailView(ExpandableFieldViewMixin, GenericAPIView): **Response Values for DELETE** Any logged in user enrolled in a course can remove themselves from - a team in the course. Course and global staff can remove any user - from a team. Successfully deleting a membership will return a 204 - response with no content. + a team in the course. Course staff, global staff, and discussion + privileged users can remove any user from a team. Successfully + deleting a membership will return a 204 response with no content. If the user is not logged in and active, a 401 error is returned. If the specified team or username does not exist, a 404 error is returned. - If the user is not staff and is attempting to remove another user - from a team, a 404 error is returned. This prevents leaking - information about team and user existence. + If the user is not staff or a discussion privileged user and is + attempting to remove another user from a team, a 404 error is + returned. This prevents leaking information about team and user + existence. If the membership does not exist, a 404 error is returned. """ From ab3b2ebd9a14cb821b3d29fe30017cf3530bdd53 Mon Sep 17 00:00:00 2001 From: Eric Fischer Date: Mon, 17 Aug 2015 17:09:25 -0400 Subject: [PATCH 2/2] Fixing problematic bok_choy test By moving the team_page.first_member_username access to occur before click_first_profile_image() is called, we avoid a potential issue where the wrapper() method defined in page_object.py, which decorates the property, calls into _verify_page() and raises a WrongPageError. --- common/test/acceptance/tests/lms/test_teams.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/common/test/acceptance/tests/lms/test_teams.py b/common/test/acceptance/tests/lms/test_teams.py index dfa521ac06..5844f0e4c9 100644 --- a/common/test/acceptance/tests/lms/test_teams.py +++ b/common/test/acceptance/tests/lms/test_teams.py @@ -954,9 +954,11 @@ class TeamPageTest(TeamsTabBase): self._set_team_configuration_and_membership() self.team_page.visit() + learner_name = self.team_page.first_member_username + self.team_page.click_first_profile_image() - learner_profile_page = LearnerProfilePage(self.browser, self.team_page.first_member_username) + learner_profile_page = LearnerProfilePage(self.browser, learner_name) learner_profile_page.wait_for_page() learner_profile_page.wait_for_field('username') self.assertTrue(learner_profile_page.field_is_visible('username'))