From cda549754bccdbcac71c5087e63c549f6630d6ee Mon Sep 17 00:00:00 2001 From: Brian Buck Date: Thu, 23 Apr 2026 18:21:58 -0600 Subject: [PATCH 1/3] fix: Update permissions for course team tab in Instructor Dashboard --- lms/djangoapps/instructor/permissions.py | 18 +++ .../instructor/tests/test_api_v2.py | 146 +++++++++++++++++- lms/djangoapps/instructor/views/api_v2.py | 9 +- .../instructor/views/serializers_v2.py | 22 +-- 4 files changed, 175 insertions(+), 20 deletions(-) diff --git a/lms/djangoapps/instructor/permissions.py b/lms/djangoapps/instructor/permissions.py index ca04c6dbc6c7..2483be906001 100644 --- a/lms/djangoapps/instructor/permissions.py +++ b/lms/djangoapps/instructor/permissions.py @@ -94,6 +94,24 @@ def has_permission(self, request, view): return request.user.has_perm(permission, course) +class CourseTeamPermission(BasePermission): + """ + Allow access to course team management endpoints for users with + instructor (Admin) access or the Discussion Admin (forum Administrator) role. + """ + def has_permission(self, request, view): + try: + course_key = CourseKey.from_string(view.kwargs.get('course_id')) + except InvalidKeyError as exc: + raise Http404('Invalid course key') from exc + course = get_course_by_id(course_key) + if has_access(request.user, 'instructor', course): + return True + if has_forum_access(request.user, course_key, FORUM_ROLE_ADMINISTRATOR): + return True + return False + + class ForumAdminRequiresInstructorAccess(BasePermission): """ default roles require either (staff & forum admin) or (instructor) diff --git a/lms/djangoapps/instructor/tests/test_api_v2.py b/lms/djangoapps/instructor/tests/test_api_v2.py index 403acb4be6a3..c6fb2d742c27 100644 --- a/lms/djangoapps/instructor/tests/test_api_v2.py +++ b/lms/djangoapps/instructor/tests/test_api_v2.py @@ -327,9 +327,9 @@ def _test_staff_tabs(self, tabs): """Helper to test tabs visible to staff users.""" tab_ids = [tab['tab_id'] for tab in tabs] - # Staff should see these basic tabs - expected_basic_tabs = ['course_info', 'enrollments', 'course_team', 'grading', 'cohorts'] - self.assertListEqual(tab_ids, expected_basic_tabs) # noqa: PT009 + # Staff should see these basic tabs (course_team is restricted to instructor/forum_admin) + expected_basic_tabs = ['course_info', 'enrollments', 'grading', 'cohorts'] + assert tab_ids == expected_basic_tabs def test_staff_sees_basic_tabs(self): """ @@ -343,7 +343,10 @@ def test_instructor_sees_all_basic_tabs(self): Test that instructors see all tabs that staff see. """ instructor_tabs = self._get_tabs_from_response(self.instructor) - self._test_staff_tabs(instructor_tabs) + tab_ids = [tab['tab_id'] for tab in instructor_tabs] + + expected_tabs = ['course_info', 'enrollments', 'course_team', 'grading', 'cohorts'] + assert tab_ids == expected_tabs def test_researcher_sees_all_basic_tabs(self): """ @@ -3369,3 +3372,138 @@ def test_revoke_forum_role(self): assert response.data['roles'] == ['Moderator'] assert response.data['action'] == 'revoke' assert not role.users.filter(pk=target.pk).exists() + + +class CourseTeamTabVisibilityTest(SharedModuleStoreTestCase): + """ + Tests that the course_team tab is only visible to Admin (instructor role) + and Discussion Admin (forum Administrator role). + + See: https://github.com/openedx/openedx-platform/issues/38439 + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create( + org='edX', + number='TabVis', + run='2024', + display_name='Tab Visibility Test Course', + ) + cls.course_key = cls.course.id + + def setUp(self): + super().setUp() + self.client = APIClient() + self.url = reverse('instructor_api_v2:course_metadata', kwargs={'course_id': str(self.course_key)}) + + # Instructor (Admin) — should see course_team tab + self.instructor = InstructorFactory.create(course_key=self.course_key) + + # Discussion Admin (forum Administrator) — should see course_team tab + self.forum_admin = StaffFactory.create(course_key=self.course_key) + seed_permissions_roles(self.course_key) + admin_role = Role.objects.get(course_id=self.course_key, name='Administrator') + admin_role.users.add(self.forum_admin) + + # Staff — should NOT see course_team tab + self.staff_user = StaffFactory.create(course_key=self.course_key) + + def _get_tab_ids(self, user): + self.client.force_authenticate(user=user) + response = self.client.get(self.url) + assert response.status_code == status.HTTP_200_OK + return [tab['tab_id'] for tab in response.data.get('tabs', [])] + + def test_instructor_sees_course_team_tab(self): + """Admin (instructor role) should see the course_team tab.""" + tab_ids = self._get_tab_ids(self.instructor) + assert 'course_team' in tab_ids + + def test_forum_admin_sees_course_team_tab(self): + """Discussion Admin (forum Administrator role) should see the course_team tab.""" + tab_ids = self._get_tab_ids(self.forum_admin) + assert 'course_team' in tab_ids + + def test_staff_does_not_see_course_team_tab(self): + """Staff without instructor or forum admin role should NOT see the course_team tab.""" + tab_ids = self._get_tab_ids(self.staff_user) + assert 'course_team' not in tab_ids + + +class CourseTeamEndpointForumAdminAccessTest(SharedModuleStoreTestCase): + """ + Tests that Discussion Admin (forum Administrator role) can access + course team endpoints, not just the instructor role. + + See: https://github.com/openedx/openedx-platform/issues/38439 + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course = CourseFactory.create( + org='edX', + number='ForumAccess', + run='2024', + display_name='Forum Admin Access Test Course', + ) + cls.course_key = cls.course.id + + def setUp(self): + super().setUp() + self.client = APIClient() + + # Discussion Admin: staff + forum Administrator role + self.forum_admin = StaffFactory.create(course_key=self.course_key) + seed_permissions_roles(self.course_key) + admin_role = Role.objects.get(course_id=self.course_key, name='Administrator') + admin_role.users.add(self.forum_admin) + + # Plain staff user (no forum admin, no instructor) — should be denied + self.staff_user = StaffFactory.create(course_key=self.course_key) + + def test_forum_admin_can_list_team_roles(self): + """Discussion Admin should be able to GET /team/roles.""" + url = reverse('instructor_api_v2:course_team_roles', kwargs={'course_id': str(self.course_key)}) + self.client.force_authenticate(user=self.forum_admin) + response = self.client.get(url) + assert response.status_code == status.HTTP_200_OK + + def test_forum_admin_can_list_team_members(self): + """Discussion Admin should be able to GET /team.""" + url = reverse('instructor_api_v2:course_team', kwargs={'course_id': str(self.course_key)}) + self.client.force_authenticate(user=self.forum_admin) + response = self.client.get(url) + assert response.status_code == status.HTTP_200_OK + + def test_forum_admin_can_grant_role(self): + """Discussion Admin should be able to POST /team to grant a role.""" + url = reverse('instructor_api_v2:course_team', kwargs={'course_id': str(self.course_key)}) + target = UserFactory.create() + self.client.force_authenticate(user=self.forum_admin) + response = self.client.post(url, { + 'identifiers': [target.username], + 'role': 'staff', + 'action': 'allow', + }, format='json') + assert response.status_code == status.HTTP_200_OK + + def test_forum_admin_can_revoke_role(self): + """Discussion Admin should be able to DELETE /team/{username}.""" + target = StaffFactory.create(course_key=self.course_key) + url = reverse( + 'instructor_api_v2:course_team_member', + kwargs={'course_id': str(self.course_key), 'email_or_username': target.username}, + ) + self.client.force_authenticate(user=self.forum_admin) + response = self.client.delete(url, {'roles': ['staff']}, format='json') + assert response.status_code == status.HTTP_200_OK + + def test_plain_staff_cannot_access_team_endpoints(self): + """Staff without instructor or forum admin role should get 403.""" + url = reverse('instructor_api_v2:course_team', kwargs={'course_id': str(self.course_key)}) + self.client.force_authenticate(user=self.staff_user) + response = self.client.get(url) + assert response.status_code == status.HTTP_403_FORBIDDEN diff --git a/lms/djangoapps/instructor/views/api_v2.py b/lms/djangoapps/instructor/views/api_v2.py index dd137f1630d4..d50d8d77893e 100644 --- a/lms/djangoapps/instructor/views/api_v2.py +++ b/lms/djangoapps/instructor/views/api_v2.py @@ -2420,8 +2420,7 @@ class CourseTeamRolesView(DeveloperErrorViewMixin, APIView): * 401: User is not authenticated * 403: User lacks instructor permissions """ - permission_classes = (IsAuthenticated, permissions.InstructorPermission) - permission_name = permissions.EDIT_COURSE_ACCESS + permission_classes = (IsAuthenticated, permissions.CourseTeamPermission) def get(self, request, course_id): """Return the list of available course team roles for this course.""" @@ -2547,8 +2546,7 @@ class CourseTeamView(DeveloperErrorViewMixin, APIView): * 403: User lacks instructor permissions * 404: Course not found """ - permission_classes = (IsAuthenticated, permissions.InstructorPermission) - permission_name = permissions.EDIT_COURSE_ACCESS + permission_classes = (IsAuthenticated, permissions.CourseTeamPermission) def get(self, request, course_id): """ @@ -2716,8 +2714,7 @@ class CourseTeamMemberView(DeveloperErrorViewMixin, APIView): * 404: Course or user not found * 409: Cannot remove own instructor access """ - permission_classes = (IsAuthenticated, permissions.InstructorPermission) - permission_name = permissions.EDIT_COURSE_ACCESS + permission_classes = (IsAuthenticated, permissions.CourseTeamPermission) def delete(self, request, course_id, email_or_username): """Revoke one or more course roles from a user.""" diff --git a/lms/djangoapps/instructor/views/serializers_v2.py b/lms/djangoapps/instructor/views/serializers_v2.py index c61f3a310eb0..d0b54bcb9967 100644 --- a/lms/djangoapps/instructor/views/serializers_v2.py +++ b/lms/djangoapps/instructor/views/serializers_v2.py @@ -168,16 +168,6 @@ def get_tabs(self, data): ), 'sort_order': 20, }, - { - 'tab_id': 'course_team', - 'title': _('Course Team'), - 'url': self._build_tab_url( - 'INSTRUCTOR_MICROFRONTEND_URL', - course_key, - 'course_team' - ), - 'sort_order': 30, - }, { 'tab_id': 'grading', 'title': _('Grading'), @@ -200,6 +190,18 @@ def get_tabs(self, data): }, ]) + if access['instructor'] or access['forum_admin']: + tabs.append({ + 'tab_id': 'course_team', + 'title': _('Course Team'), + 'url': self._build_tab_url( + 'INSTRUCTOR_MICROFRONTEND_URL', + course_key, + 'course_team' + ), + 'sort_order': 30, + }) + if access['staff'] and is_bulk_email_feature_enabled(course_key): tabs.append( { From 907a52485e62ef2e4d63339c92948085288435ba Mon Sep 17 00:00:00 2001 From: Brian Buck Date: Thu, 23 Apr 2026 20:06:18 -0600 Subject: [PATCH 2/3] fix: Copilot PR feedback --- lms/djangoapps/instructor/permissions.py | 6 ++++-- lms/djangoapps/instructor/tests/test_api_v2.py | 11 +++++++++++ lms/djangoapps/instructor/views/api_v2.py | 6 +++--- lms/djangoapps/instructor/views/serializers_v2.py | 2 +- 4 files changed, 19 insertions(+), 6 deletions(-) diff --git a/lms/djangoapps/instructor/permissions.py b/lms/djangoapps/instructor/permissions.py index 2483be906001..baa36bddd5a4 100644 --- a/lms/djangoapps/instructor/permissions.py +++ b/lms/djangoapps/instructor/permissions.py @@ -97,7 +97,7 @@ def has_permission(self, request, view): class CourseTeamPermission(BasePermission): """ Allow access to course team management endpoints for users with - instructor (Admin) access or the Discussion Admin (forum Administrator) role. + instructor (Admin) access or the Discussion Admin (staff + forum Administrator) role. """ def has_permission(self, request, view): try: @@ -107,7 +107,9 @@ def has_permission(self, request, view): course = get_course_by_id(course_key) if has_access(request.user, 'instructor', course): return True - if has_forum_access(request.user, course_key, FORUM_ROLE_ADMINISTRATOR): + if has_access(request.user, 'staff', course) and has_forum_access( + request.user, course_key, FORUM_ROLE_ADMINISTRATOR + ): return True return False diff --git a/lms/djangoapps/instructor/tests/test_api_v2.py b/lms/djangoapps/instructor/tests/test_api_v2.py index c6fb2d742c27..b8b833a4b74b 100644 --- a/lms/djangoapps/instructor/tests/test_api_v2.py +++ b/lms/djangoapps/instructor/tests/test_api_v2.py @@ -3507,3 +3507,14 @@ def test_plain_staff_cannot_access_team_endpoints(self): self.client.force_authenticate(user=self.staff_user) response = self.client.get(url) assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_non_staff_forum_admin_cannot_access_team_endpoints(self): + """Non-staff user with only forum Administrator role should get 403.""" + non_staff_forum_admin = UserFactory.create() + admin_role = Role.objects.get(course_id=self.course_key, name='Administrator') + admin_role.users.add(non_staff_forum_admin) + + url = reverse('instructor_api_v2:course_team', kwargs={'course_id': str(self.course_key)}) + self.client.force_authenticate(user=non_staff_forum_admin) + response = self.client.get(url) + assert response.status_code == status.HTTP_403_FORBIDDEN diff --git a/lms/djangoapps/instructor/views/api_v2.py b/lms/djangoapps/instructor/views/api_v2.py index d50d8d77893e..fa50fcf8c41a 100644 --- a/lms/djangoapps/instructor/views/api_v2.py +++ b/lms/djangoapps/instructor/views/api_v2.py @@ -2418,7 +2418,7 @@ class CourseTeamRolesView(DeveloperErrorViewMixin, APIView): * 200: OK * 401: User is not authenticated - * 403: User lacks instructor permissions + * 403: User lacks course team management permissions (requires instructor or discussion Administrator role) """ permission_classes = (IsAuthenticated, permissions.CourseTeamPermission) @@ -2543,7 +2543,7 @@ class CourseTeamView(DeveloperErrorViewMixin, APIView): * 200: OK (GET, POST - role granted/revoked) * 400: Invalid parameters * 401: User is not authenticated - * 403: User lacks instructor permissions + * 403: User lacks course team management permissions (requires instructor or discussion Administrator role) * 404: Course not found """ permission_classes = (IsAuthenticated, permissions.CourseTeamPermission) @@ -2710,7 +2710,7 @@ class CourseTeamMemberView(DeveloperErrorViewMixin, APIView): * 200: Role(s) revoked successfully * 400: Invalid parameters * 401: User is not authenticated - * 403: User lacks instructor permissions + * 403: User lacks course team management permissions (requires instructor or discussion Administrator role) * 404: Course or user not found * 409: Cannot remove own instructor access """ diff --git a/lms/djangoapps/instructor/views/serializers_v2.py b/lms/djangoapps/instructor/views/serializers_v2.py index d0b54bcb9967..415f948d75ae 100644 --- a/lms/djangoapps/instructor/views/serializers_v2.py +++ b/lms/djangoapps/instructor/views/serializers_v2.py @@ -190,7 +190,7 @@ def get_tabs(self, data): }, ]) - if access['instructor'] or access['forum_admin']: + if access['instructor'] or (access['staff'] and access['forum_admin']): tabs.append({ 'tab_id': 'course_team', 'title': _('Course Team'), From e0879c4b13d13526d1160aca58605b1c4b6b6f6e Mon Sep 17 00:00:00 2001 From: Brian Buck Date: Fri, 24 Apr 2026 08:40:46 -0600 Subject: [PATCH 3/3] fix: PR feedback regarding permissions elevation bug --- .../instructor/tests/test_api_v2.py | 24 +++++++++++++++++++ lms/djangoapps/instructor/views/api_v2.py | 12 ++++++++++ 2 files changed, 36 insertions(+) diff --git a/lms/djangoapps/instructor/tests/test_api_v2.py b/lms/djangoapps/instructor/tests/test_api_v2.py index b8b833a4b74b..fd8008d48849 100644 --- a/lms/djangoapps/instructor/tests/test_api_v2.py +++ b/lms/djangoapps/instructor/tests/test_api_v2.py @@ -3518,3 +3518,27 @@ def test_non_staff_forum_admin_cannot_access_team_endpoints(self): self.client.force_authenticate(user=non_staff_forum_admin) response = self.client.get(url) assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_forum_admin_cannot_grant_instructor_role(self): + """Discussion Admin should not be able to grant the instructor role (privilege escalation).""" + url = reverse('instructor_api_v2:course_team', kwargs={'course_id': str(self.course_key)}) + target = UserFactory.create() + self.client.force_authenticate(user=self.forum_admin) + response = self.client.post(url, { + 'identifiers': [target.username], + 'role': 'instructor', + 'action': 'allow', + }, format='json') + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_forum_admin_cannot_revoke_instructor_role(self): + """Discussion Admin should not be able to revoke the instructor role.""" + # Create an instructor to target + instructor = InstructorFactory.create(course_key=self.course_key) + url = reverse( + 'instructor_api_v2:course_team_member', + kwargs={'course_id': str(self.course_key), 'email_or_username': instructor.username}, + ) + self.client.force_authenticate(user=self.forum_admin) + response = self.client.delete(url, {'roles': ['instructor']}, format='json') + assert response.status_code == status.HTTP_403_FORBIDDEN diff --git a/lms/djangoapps/instructor/views/api_v2.py b/lms/djangoapps/instructor/views/api_v2.py index fa50fcf8c41a..0af6ec13dc6f 100644 --- a/lms/djangoapps/instructor/views/api_v2.py +++ b/lms/djangoapps/instructor/views/api_v2.py @@ -2631,6 +2631,12 @@ def post(self, request, course_id): rolename = serializer.validated_data['role'] action = serializer.validated_data['action'] + if rolename == 'instructor' and not has_access(request.user, 'instructor', course): + return Response( + {'error': _('Managing the instructor role requires instructor access.')}, + status=status.HTTP_403_FORBIDDEN, + ) + results = [] for identifier in identifiers: error = False @@ -2727,6 +2733,12 @@ def delete(self, request, course_id, email_or_username): roles = revoke_serializer.validated_data['roles'] + if 'instructor' in roles and not has_access(request.user, 'instructor', course): + return Response( + {'error': _('Managing the instructor role requires instructor access.')}, + status=status.HTTP_403_FORBIDDEN, + ) + try: user = get_student_from_identifier(email_or_username) except User.DoesNotExist: