Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions cms/djangoapps/contentstore/rest_api/v2/serializers/home.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
18 changes: 14 additions & 4 deletions cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}'),
Expand All @@ -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)}'),
Expand Down Expand Up @@ -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)}'),
Expand Down Expand Up @@ -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)}'),
Expand Down Expand Up @@ -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)}'),
Expand Down
55 changes: 47 additions & 8 deletions lms/djangoapps/support/rest_api/v1/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)

Expand All @@ -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)

Expand Down Expand Up @@ -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
)
Expand All @@ -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 ---

Expand Down
34 changes: 32 additions & 2 deletions lms/djangoapps/support/rest_api/v1/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand All @@ -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(
Expand Down Expand Up @@ -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):
Expand Down
Loading