From 9707dcff6eda783b4a633cab08b1cc4ea7c93f38 Mon Sep 17 00:00:00 2001
From: Jansen Kantor <jkantor@edx.org>
Date: Fri, 15 May 2020 16:50:31 -0400
Subject: [PATCH] EDUCATOR 5042: Learner's Private Team Appears When Learner
Browses all Team Sets (#24004)
* fix query to remove dulicates and incorrect teams
---
lms/djangoapps/teams/tests/test_views.py | 37 +++++++++++++++++++
lms/djangoapps/teams/views.py | 47 ++++++++++++------------
2 files changed, 60 insertions(+), 24 deletions(-)
diff --git a/lms/djangoapps/teams/tests/test_views.py b/lms/djangoapps/teams/tests/test_views.py
index 20aedd435b5..4c1290245d6 100644
--- a/lms/djangoapps/teams/tests/test_views.py
+++ b/lms/djangoapps/teams/tests/test_views.py
@@ -966,6 +966,43 @@ class TestListTeamsAPI(EventTestMixin, TeamAPITestCase):
user='staff'
)
+ def test_duplicates_and_nontopic_private_teamsets(self):
+ """
+ Test for a bug where non-admin users would have their private memberships returned from this endpoint
+ despite the topic, and duplicate entries for teams in the topic that was being queried (EDUCATOR-5042)
+ """
+ # create a team in a private teamset and add a user
+ unprotected_team_in_private_teamset = CourseTeamFactory.create(
+ name='unprotected_team_in_private_teamset',
+ description='unprotected_team_in_private_teamset',
+ course_id=self.test_course_1.id,
+ topic_id='private_topic_1_id',
+ )
+ unprotected_team_in_private_teamset.add_user(self.users['student_enrolled'])
+
+ # make some more users and put them in the solar team.
+ another_student_username = 'another_student'
+ yet_another_student_username = 'yet_another_student'
+ self.create_and_enroll_student(username=another_student_username)
+ self.create_and_enroll_student(username=yet_another_student_username)
+ self.solar_team.add_user(self.users[another_student_username])
+ self.solar_team.add_user(self.users[yet_another_student_username])
+
+ teams = self.get_teams_list(data={'topic_id': self.solar_team.topic_id}, user='student_enrolled')
+ team_names = [team['name'] for team in teams['results']]
+ team_names.sort()
+ self.assertEqual(team_names, [
+ self.solar_team.name,
+ ])
+
+ teams = self.get_teams_list(data={'topic_id': self.solar_team.topic_id}, user='staff')
+ team_names = [team['name'] for team in teams['results']]
+ team_names.sort()
+ self.assertEqual(team_names, [
+ self.solar_team.name,
+ self.masters_only_team.name,
+ ])
+
@ddt.ddt
class TestCreateTeamAPI(EventTestMixin, TeamAPITestCase):
diff --git a/lms/djangoapps/teams/views.py b/lms/djangoapps/teams/views.py
index fe2f9fa9054..8a8cc03f45a 100644
--- a/lms/djangoapps/teams/views.py
+++ b/lms/djangoapps/teams/views.py
@@ -470,22 +470,11 @@ class TeamsListView(ExpandableFieldViewMixin, GenericAPIView):
# Non-staff users should not be able to see private_managed teams that they are not on.
# Staff shouldn't have any excluded teams.
-
- if not has_access(request.user, 'staff', course_key):
- private_teamset_ids = [ts.teamset_id for ts in course_module.teamsets if ts.is_private_managed]
- excluded_team_ids = CourseTeam.objects.filter(
- course_id=course_key,
- topic_id__in=private_teamset_ids
- ).exclude(
- membership__user=request.user
- ).values_list('team_id', flat=True)
- excluded_team_ids = set(excluded_team_ids)
- else:
- excluded_team_ids = set()
+ excluded_private_team_ids = self._get_private_team_ids_to_exclude(course_module)
search_results['results'] = [
result for result in search_results['results']
- if result['data']['id'] not in excluded_team_ids
+ if result['data']['id'] not in excluded_private_team_ids
]
search_results['total'] = len(search_results['results'])
@@ -511,18 +500,10 @@ class TeamsListView(ExpandableFieldViewMixin, GenericAPIView):
'last_activity_at': ('-last_activity_at', 'team_size'),
}
- if not has_access(request.user, 'staff', course_key):
- # hide private_managed courses from non-admin users that aren't members of those teams
- private_topic_ids = [ts.teamset_id for ts in course_module.teamsets if
- ts.is_private_managed]
- public_teams = CourseTeam.objects.filter(**result_filter).exclude(
- topic_id__in=private_topic_ids)
- private_managed_teams_of_user = CourseTeam.objects.filter(topic_id__in=private_topic_ids,
- membership__user__username=request.user)
- queryset = public_teams | private_managed_teams_of_user
- else:
- queryset = CourseTeam.objects.filter(**result_filter)
+ # hide private_managed courses from non-staff users that aren't members of those teams
+ excluded_private_team_ids = self._get_private_team_ids_to_exclude(course_module)
+ queryset = CourseTeam.objects.filter(**result_filter).exclude(team_id__in=excluded_private_team_ids)
order_by_input = request.query_params.get('order_by', 'name')
if order_by_input not in ordering_schemes:
return Response(
@@ -651,6 +632,24 @@ class TeamsListView(ExpandableFieldViewMixin, GenericAPIView):
page_query_param = self.request.query_params.get(self.paginator.page_query_param)
return page_kwarg or page_query_param or 1
+ def _get_private_team_ids_to_exclude(self, course_module):
+ """
+ Get the list of team ids that should be excluded from the response.
+ Staff can see all private teams.
+ Users should not be able to see teams in private teamsets that they are not a member of.
+ """
+ if has_access(self.request.user, 'staff', course_module.id):
+ return set()
+
+ private_teamset_ids = [ts.teamset_id for ts in course_module.teamsets if ts.is_private_managed]
+ excluded_team_ids = CourseTeam.objects.filter(
+ course_id=course_module.id,
+ topic_id__in=private_teamset_ids
+ ).exclude(
+ membership__user=self.request.user
+ ).values_list('team_id', flat=True)
+ return set(excluded_team_ids)
+
class IsEnrolledOrIsStaff(permissions.BasePermission):
"""Permission that checks to see if the user is enrolled in the course or is staff."""
--
GitLab