diff --git a/lms/djangoapps/instructor/permissions.py b/lms/djangoapps/instructor/permissions.py index ca04c6dbc6c7..baa36bddd5a4 100644 --- a/lms/djangoapps/instructor/permissions.py +++ b/lms/djangoapps/instructor/permissions.py @@ -94,6 +94,26 @@ 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 (staff + 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_access(request.user, 'staff', course) and 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..fd8008d48849 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,173 @@ 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 + + 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 + + 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 dd137f1630d4..0af6ec13dc6f 100644 --- a/lms/djangoapps/instructor/views/api_v2.py +++ b/lms/djangoapps/instructor/views/api_v2.py @@ -2418,10 +2418,9 @@ 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.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.""" @@ -2544,11 +2543,10 @@ 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.InstructorPermission) - permission_name = permissions.EDIT_COURSE_ACCESS + permission_classes = (IsAuthenticated, permissions.CourseTeamPermission) def get(self, request, course_id): """ @@ -2633,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 @@ -2712,12 +2716,11 @@ 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 """ - 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.""" @@ -2730,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: diff --git a/lms/djangoapps/instructor/views/serializers_v2.py b/lms/djangoapps/instructor/views/serializers_v2.py index c61f3a310eb0..415f948d75ae 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['staff'] and 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( {