diff --git a/lms/djangoapps/teams/api.py b/lms/djangoapps/teams/api.py index 4f0a3a5c1f..c2b82b84ec 100644 --- a/lms/djangoapps/teams/api.py +++ b/lms/djangoapps/teams/api.py @@ -193,6 +193,33 @@ def has_specific_team_access(user, team): ) +def has_specific_teamset_access(user, course_module, teamset_id): + """ + Staff have access to all teamsets. + All non-staff users have access to open and public_managed teamsets. + Non-staff users only have access to a private_managed teamset if they are in a team in that teamset + """ + return has_course_staff_privileges(user, course_module.id) or \ + teamset_is_public_or_user_is_on_team_in_teamset(user, course_module, teamset_id) + + +def teamset_is_public_or_user_is_on_team_in_teamset(user, course_module, teamset_id): + """ + The only users who should be able to see private_managed teamsets + or recieve any information about them at all from the API are: + - Course staff + - Users who are enrolled in a team in a private_managed teamset + + course_module is passed in because almost universally where we'll be calling this, we will already + need to have looked up the course from modulestore to make sure that the topic we're interested in + exists in the course. + """ + teamset = course_module.teams_configuration.teamsets_by_id[teamset_id] + if teamset.teamset_type != TeamsetType.private_managed: + return True + return CourseTeamMembership.user_in_team_for_course(user, course_module.id, topic_id=teamset_id) + + def user_on_team_or_team_is_public(user, team): """ The only users who should be able to see private_managed teams diff --git a/lms/djangoapps/teams/tests/test_views.py b/lms/djangoapps/teams/tests/test_views.py index 3fcb1d5935..4d0f8c86fc 100644 --- a/lms/djangoapps/teams/tests/test_views.py +++ b/lms/djangoapps/teams/tests/test_views.py @@ -594,7 +594,14 @@ class TeamAPITestCase(APITestCase, SharedModuleStoreTestCase): team_list = self.get_teams_list(user=user, expected_status=200, data=course_two_data) self.assertEqual(team_list['count'], 0) - def build_team_data(self, name="Test team", course=None, description="Filler description", **kwargs): + def build_team_data( + self, + name="Test team", + course=None, + description="Filler description", + topic_id="topic_0", + **kwargs + ): """Creates the payload for creating a team. kwargs can be used to specify additional fields.""" data = kwargs course = course if course else self.test_course_1 @@ -602,6 +609,7 @@ class TeamAPITestCase(APITestCase, SharedModuleStoreTestCase): 'name': name, 'course_id': str(course.id), 'description': description, + 'topic_id': topic_id, }) return data @@ -1008,7 +1016,19 @@ class TestCreateTeamAPI(EventTestMixin, TeamAPITestCase): def test_bad_course_data(self, status, data): self.post_create_team(status, data) - def test_student_in_team(self): + def test_bad_topic_id(self): + self.post_create_team( + 404, + data=self.build_team_data(topic_id='asdfasdfasdfa'), + user='staff' + ) + + def test_missing_topic_id(self): + data = self.build_team_data() + data.pop('topic_id') + self.post_create_team(400, data=data, user='staff') + + def test_student_in_teamset(self): response = self.post_create_team( 400, data=self.build_team_data( @@ -1019,11 +1039,12 @@ class TestCreateTeamAPI(EventTestMixin, TeamAPITestCase): user='student_enrolled' ) self.assertEqual( - "You are already in a team in this course.", + "You are already in a team in this teamset.", json.loads(response.content.decode('utf-8'))["user_message"] ) @patch('lms.djangoapps.teams.views.can_user_create_team_in_topic', return_value=False) + @patch('lms.djangoapps.teams.views.has_specific_teamset_access', return_value=True) def test_student_create_team_instructor_managed_topic(self, *args): # pylint: disable=unused-argument response = self.post_create_team( 403, @@ -1031,7 +1052,7 @@ class TestCreateTeamAPI(EventTestMixin, TeamAPITestCase): name="student create team in instructor managed topic", course=self.test_course_1, description="student cannot create team in instructor-managed topic", - topic_id='great-topic' + topic_id='private_topic_1_id' ), user='student_enrolled_not_on_team' ) @@ -1054,7 +1075,8 @@ class TestCreateTeamAPI(EventTestMixin, TeamAPITestCase): data=self.build_team_data( name="Another team", course=self.test_course_1, - description="Privileged users are the best" + description="Privileged users are the best", + topic_id=self.solar_team.topic_id ), user=user ) @@ -1139,12 +1161,12 @@ class TestCreateTeamAPI(EventTestMixin, TeamAPITestCase): @ddt.unpack @ddt.data( - ('student_enrolled', 400, 'You are already in a team in this course.'), + ('student_enrolled', 404, None), ('student_unenrolled', 403, None), - ('student_enrolled_not_on_team', 403, "You can't create a team in an instructor managed topic."), - ('student_masters', 400, 'You are already in a team in this course.'), - ('student_on_team_1_private_set_1', 400, 'You are already in a team in this course.'), - ('student_on_team_2_private_set_1', 400, 'You are already in a team in this course.'), + ('student_enrolled_not_on_team', 404, None), + ('student_masters', 404, None), + ('student_on_team_1_private_set_1', 403, "You can't create a team in an instructor managed topic."), + ('student_on_team_2_private_set_1', 403, "You can't create a team in an instructor managed topic."), ('staff', 200, None) ) def test_private_managed_access(self, user, expected_response, msg): @@ -1153,10 +1175,6 @@ class TestCreateTeamAPI(EventTestMixin, TeamAPITestCase): Only staff should be able to create teams in managed teamsets, but they're also the only ones who should know that private_managed teamsets exist. If the team hasn't been created yet, no one can be in it, so no non-staff should get any info at all from this endpoint. - - TODO: This endpoint should check for teamset enrollment rather than course enrollment. - I'm not sure of the severity of the 'user_message' info, but it seems like we might not want to - return that for student_enrolled_not_on_team (they should just get 403, like student_unenrolled) """ response = self.post_create_team( expected_response, diff --git a/lms/djangoapps/teams/views.py b/lms/djangoapps/teams/views.py index 4f15cf3c45..30bb976904 100644 --- a/lms/djangoapps/teams/views.py +++ b/lms/djangoapps/teams/views.py @@ -53,6 +53,7 @@ from .api import ( can_user_create_team_in_topic, has_course_staff_privileges, has_specific_team_access, + has_specific_teamset_access, has_team_api_access, user_organization_protection_status ) @@ -363,7 +364,7 @@ class TeamsListView(ExpandableFieldViewMixin, GenericAPIView): **Response Values for POST** Any logged in user who has verified their email address can create - a team. The format mirrors that of a GET for an individual team, + a team in an open teamset. The format mirrors that of a GET for an individual team, but does not include the id, date_created, or membership fields. id is automatically computed based on name. @@ -373,10 +374,13 @@ class TeamsListView(ExpandableFieldViewMixin, GenericAPIView): 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. + If the course_id is not valid, or the topic_id is missing, or extra fields + are included in the request, a 400 error is returned. If the specified course does not exist, a 404 error is returned. + If the specified teamset does not exist, a 404 error is returned. + If the specified teamset does exist, but the requesting user shouldn't be + able to see it, a 404 is returned. """ # BearerAuthentication must come first to return a 401 for unauthenticated users @@ -547,12 +551,13 @@ class TeamsListView(ExpandableFieldViewMixin, GenericAPIView): """POST /api/team/v0/teams/""" field_errors = {} course_key = None - course_id = request.data.get('course_id') + #Handle field errors and check that the course exists try: course_key = CourseKey.from_string(course_id) # Ensure the course exists - if not modulestore().has_course(course_key): + course_module = modulestore().get_course(course_key) + if not course_module: return Response(status=status.HTTP_404_NOT_FOUND) except InvalidKeyError: field_errors['course_id'] = build_api_error( @@ -563,27 +568,46 @@ class TeamsListView(ExpandableFieldViewMixin, GenericAPIView): 'field_errors': field_errors, }, status=status.HTTP_400_BAD_REQUEST) - # Course and global staff, as well as discussion "privileged" users, will not automatically - # be added to a team when they create it. They are allowed to create multiple teams. - team_administrator = (has_access(request.user, 'staff', course_key) - or has_discussion_privileges(request.user, course_key)) - if not team_administrator and CourseTeamMembership.user_in_team_for_course(request.user, course_key): - error_message = build_api_error( - ugettext_noop('You are already in a team in this course.'), + topic_id = request.data.get('topic_id') + if not topic_id: + field_errors['topic_id'] = build_api_error( + ugettext_noop(u'topic_id is required'), course_id=course_id ) - return Response(error_message, status=status.HTTP_400_BAD_REQUEST) + return Response({ + 'field_errors': field_errors, + }, status=status.HTTP_400_BAD_REQUEST) if course_key and not has_team_api_access(request.user, course_key): return Response(status=status.HTTP_403_FORBIDDEN) - topic_id = request.data.get('topic_id') + if topic_id not in course_module.teams_configuration.teamsets_by_id or ( + not has_specific_teamset_access(request.user, course_module, topic_id) + ): + return Response(status=status.HTTP_404_NOT_FOUND) + + # The user has to have access to this teamset at this point, so we can return 403 + # and not leak the existance of a private teamset if not can_user_create_team_in_topic(request.user, course_key, topic_id): return Response( build_api_error(ugettext_noop("You can't create a team in an instructor managed topic.")), status=status.HTTP_403_FORBIDDEN ) + # Course and global staff, as well as discussion "privileged" users, will not automatically + # be added to a team when they create it. They are allowed to create multiple teams. + is_team_administrator = (has_access(request.user, 'staff', course_key) + or has_discussion_privileges(request.user, course_key)) + if not is_team_administrator and ( + CourseTeamMembership.user_in_team_for_course(request.user, course_key, topic_id=topic_id) + ): + error_message = build_api_error( + ugettext_noop('You are already in a team in this teamset.'), + course_id=course_id, + teamset_id=topic_id, + ) + return Response(error_message, status=status.HTTP_400_BAD_REQUEST) + data = request.data.copy() data['course_id'] = six.text_type(course_key) @@ -603,7 +627,7 @@ class TeamsListView(ExpandableFieldViewMixin, GenericAPIView): emit_team_event('edx.team.created', course_key, { 'team_id': team.team_id }) - if not team_administrator: + if not is_team_administrator: # Add the creating user to the team. team.add_user(request.user) emit_team_event(