diff --git a/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py b/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py index 857291fe838b..efe65a94c783 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/serializers/home.py @@ -30,7 +30,9 @@ class CourseCommonSerializerV2(serializers.Serializer): lms_link = serializers.SerializerMethodField() cms_link = serializers.SerializerMethodField() number = serializers.CharField() + display_number = serializers.CharField(source='display_number_with_default') org = serializers.CharField() + display_org = serializers.CharField(source='display_org_with_default') rerun_link = serializers.SerializerMethodField() run = serializers.CharField(source='id.run') url = serializers.SerializerMethodField() diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py index 6a51610ac9f2..453f69ae4225 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py @@ -58,8 +58,10 @@ def test_home_page_response(self): ("display_name", self.course.display_name), ("lms_link", f'{settings.LMS_ROOT_URL}/courses/{course_id}/jump_to/{self.course.location}'), ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.course.id)}'), - ("number", self.course.number), - ("org", self.course.org), + ("number", self.active_course.number), + ("display_number", self.active_course.display_number_with_default), + ("org", self.active_course.org), + ("display_org", self.active_course.display_org_with_default), ("rerun_link", f'/course_rerun/{course_id}'), ("run", self.course.id.run), ("url", f'/course/{course_id}'), @@ -77,7 +79,9 @@ def test_home_page_response(self): f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}', ), ("number", self.archived_course.number), + ("display_number", self.archived_course.display_number_with_default), ("org", self.archived_course.org), + ("display_org", self.archived_course.display_org_with_default), ("rerun_link", f'/course_rerun/{str(self.archived_course.id)}'), ("run", self.archived_course.id.run), ("url", f'/course/{str(self.archived_course.id)}'), @@ -111,8 +115,10 @@ def test_active_only_query_if_passed(self): ("display_name", self.course.display_name), ("lms_link", f'{settings.LMS_ROOT_URL}/courses/{str(self.course.id)}/jump_to/{self.course.location}'), ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.course.id)}'), - ("number", self.course.number), - ("org", self.course.org), + ("number", self.active_course.number), + ("display_number", self.active_course.display_number_with_default), + ("org", self.active_course.org), + ("display_org", self.active_course.display_org_with_default), ("rerun_link", f'/course_rerun/{str(self.course.id)}'), ("run", self.course.id.run), ("url", f'/course/{str(self.course.id)}'), @@ -142,7 +148,9 @@ def test_archived_only_query_if_passed(self): ), ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), ("number", self.archived_course.number), + ("display_number", self.archived_course.display_number_with_default), ("org", self.archived_course.org), + ("display_org", self.archived_course.display_org_with_default), ("rerun_link", f'/course_rerun/{str(self.archived_course.id)}'), ("run", self.archived_course.id.run), ("url", f'/course/{str(self.archived_course.id)}'), @@ -172,7 +180,9 @@ def test_search_query_if_passed(self): ), ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), ("number", self.archived_course.number), + ("display_number", self.archived_course.display_number_with_default), ("org", self.archived_course.org), + ("display_org", self.archived_course.display_org_with_default), ("rerun_link", f'/course_rerun/{str(self.archived_course.id)}'), ("run", self.archived_course.id.run), ("url", f'/course/{str(self.archived_course.id)}'), diff --git a/lms/djangoapps/support/rest_api/v1/tests/test_views.py b/lms/djangoapps/support/rest_api/v1/tests/test_views.py index 58de5c965ecc..f4567b101426 100644 --- a/lms/djangoapps/support/rest_api/v1/tests/test_views.py +++ b/lms/djangoapps/support/rest_api/v1/tests/test_views.py @@ -8,7 +8,7 @@ from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.student.models.user import CourseAccessRole -from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole +from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole, SupportStaffRole from common.djangoapps.student.tests.factories import ( TEST_PASSWORD, AdminFactory, @@ -55,7 +55,7 @@ def setUp(self): def test_get_api_missing_query_params_returns_400(self): """GET API: Returns 400 if no query parameters are provided.""" - self.client.login(username=self.user.username, password=TEST_PASSWORD) + self.client.login(username=self.staff.username, password=TEST_PASSWORD) resp = self.client.get(self.url) self.assertEqual(resp.status_code, 400) @@ -75,7 +75,7 @@ def test_get_api_with_user_id_parameter(self): def test_get_api_nonexistent_user_returns_404(self): """GET API: Returns 404 for a nonexistent user email.""" - self.client.login(username=self.user.username, password=TEST_PASSWORD) + self.client.login(username=self.staff.username, password=TEST_PASSWORD) resp = self.client.get(self.url, {"email": "notfound@example.com"}) self.assertEqual(resp.status_code, 404) @@ -119,7 +119,13 @@ def test_get_api_admin_can_fetch_course_roles(self, assigned_role, expected_role self.assertTrue(course_found, "Expected course not found in response.") def test_get_api_instructor_can_only_see_their_courses(self): - """GET API: Course instructor sees only courses they have access to.""" + """ + GET API: Course instructor sees only courses within their scope. + + Read access mirrors PUT: any user with an instructor CourseAccessRole + may reach GET so they can inspect the current state before making + modifications. + """ self.client.login( username=self.instructor_user.username, password=TEST_PASSWORD ) @@ -131,12 +137,45 @@ def test_get_api_instructor_can_only_see_their_courses(self): for i in range(1, 3): self.assertNotIn(str(self.extra_courses[i].id), course_ids) - def test_get_api_user_with_no_access_sees_no_courses(self): - """GET API: Non-instructor users see no courses in the response.""" + def test_get_api_plain_learner_returns_403(self): + """ + GET API: A plain authenticated learner is denied. + + Regression for GHSA-95xv-3c54-c3pw: before the fix, this call reached + the view and produced a 200 with an empty results list, letting any + authenticated user enumerate other accounts via the 404-vs-200 response. + """ self.client.login(username=self.user.username, password=TEST_PASSWORD) resp = self.client.get(self.url, {"email": self.user.email}) - self.assertEqual(resp.status_code, 200) - self.assertEqual(resp.data, []) + self.assertEqual(resp.status_code, 403) # noqa: PT009 + + def test_get_api_enumeration_not_reachable_by_learner(self): + """ + GET API: A plain learner cannot enumerate accounts via 404 vs 200. + + Regression for GHSA-95xv-3c54-c3pw: the fix must return 403 for BOTH an + existing target and a nonexistent target so the response doesn't confirm + whether an arbitrary email/username/user_id belongs to an active account. + """ + self.client.login(username=self.user.username, password=TEST_PASSWORD) + existing = self.client.get(self.url, {"email": self.user.email}) + absent = self.client.get(self.url, {"email": "notfound@example.com"}) + self.assertEqual(existing.status_code, 403) # noqa: PT009 + self.assertEqual(absent.status_code, 403) # noqa: PT009 + + def test_get_api_support_role_sees_all_courses(self): + """ + GET API: A user holding SupportStaffRole reaches the view and sees all + courses (like admin/staff), not the empty list they'd get by falling + through to the instructor branch. + """ + SupportStaffRole().add_users(self.user) + self.client.login(username=self.user.username, password=TEST_PASSWORD) + resp = self.client.get(self.url, {"email": self.user.email}) + self.assertEqual(resp.status_code, 200) # noqa: PT009 + course_ids = [course["course_id"] for course in resp.data] + for extra in self.extra_courses: + self.assertIn(str(extra.id), course_ids) # noqa: PT009 # --- PUT API TEST CASES --- diff --git a/lms/djangoapps/support/rest_api/v1/views.py b/lms/djangoapps/support/rest_api/v1/views.py index 73bbdf04da62..ebac8147a23a 100644 --- a/lms/djangoapps/support/rest_api/v1/views.py +++ b/lms/djangoapps/support/rest_api/v1/views.py @@ -7,13 +7,14 @@ from django.db.models import Q from opaque_keys.edx.keys import CourseKey from rest_framework import status -from rest_framework.exceptions import NotFound, ValidationError +from rest_framework.exceptions import NotFound, PermissionDenied, ValidationError from rest_framework.generics import GenericAPIView from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.student.models.user import CourseAccessRole +from common.djangoapps.student.roles import SupportStaffRole from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from ..serializers import CourseTeamManageSerializer @@ -45,6 +46,27 @@ def get_serializer_context(self): context["course_role_map"] = getattr(self, "_course_role_map", {}) return context + def _caller_can_manage_course_team(self, auth_user): + """ + Return True if the caller may view or manage course team roles. + + This is the union of the PUT authorization set (`is_staff`, + `is_superuser`, or any user with a `CourseAccessRole` of + `role="instructor"`) and the global `SupportStaffRole`. Read access + must at least match write access — a user who can PUT changes needs + to see the current state to make them — and the endpoint lives in + the support module, so support staff belong in the set too. + """ + if ( + auth_user.is_superuser + or auth_user.is_staff + or SupportStaffRole().has_user(auth_user) + ): + return True + return CourseAccessRole.objects.filter( + user=auth_user, role="instructor" + ).exists() + def get_course_role_map_for_user(self, user): """Return a mapping of course_id to role for staff/instructor roles of given user.""" access_roles = CourseAccessRole.objects.filter( @@ -60,7 +82,11 @@ def get_course_role_map_for_user(self, user): def get_accessible_courses_for_user(self, auth_user): """Return queryset of courses accessible by the authenticated user.""" - if auth_user.is_superuser or auth_user.is_staff: + if ( + auth_user.is_superuser + or auth_user.is_staff + or SupportStaffRole().has_user(auth_user) + ): return CourseOverview.objects.all() access_roles = CourseAccessRole.objects.filter( @@ -217,6 +243,10 @@ def get(self, request, *args, **kwargs): } ``` """ + if not self._caller_can_manage_course_team(request.user): + raise PermissionDenied( + "You do not have permission to view course team information." + ) return self.list(request, *args, **kwargs) def put(self, request, *args, **kwargs):