From 3f2eb1529e4b3bad56c5475cda4ca56c37a14155 Mon Sep 17 00:00:00 2001 From: kingoftech-v01 Date: Fri, 10 Apr 2026 11:14:46 +0000 Subject: [PATCH] fix(api): correct IsUserInUrlOrStaff permission class to return boolean IsUserInUrlOrStaff.has_permission returned the expression `IsStaff | IsUserInUrl`, which under DRF 3.9+ evaluates to a composite permission class object rather than a boolean. A class object is always truthy, so the permission check unconditionally granted access to any authenticated caller regardless of whether the request user matched the username in the URL or held staff status. The defect was introduced in PR #28663 (Oct 2021, commit 9e787a09bc) when the dependency on rest_condition was removed: `C(IsStaff) | IsUserInUrl` was replaced with `IsStaff | IsUserInUrl` under the assumption that native DRF composition would apply, but native composition only works at the view-level permission_classes attribute, not inside a has_permission method body. Exploitation was prevented in practice by a defense-in-depth check in openedx/core/djangoapps/user_api/preferences/api.py::_check_authorized, which independently rejects cross-user access. This fix restores the intended behaviour at the DRF layer so the API no longer relies solely on the internal check. Instantiate IsStaff and IsUserInUrl and delegate to their has_permission methods, OR-ing the boolean results. Adds regression tests for staff, owner-matching, mismatched, and anonymous cases. --- openedx/core/lib/api/permissions.py | 13 +++- .../core/lib/api/tests/test_permissions.py | 64 ++++++++++++++++++- 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/openedx/core/lib/api/permissions.py b/openedx/core/lib/api/permissions.py index 127c3c5d00ab..e0f8dcb1294e 100644 --- a/openedx/core/lib/api/permissions.py +++ b/openedx/core/lib/api/permissions.py @@ -101,8 +101,19 @@ def has_permission(self, request, view): class IsUserInUrlOrStaff(permissions.BasePermission): + """ + Permission that grants access if the requesting user matches the + username in the URL or is global staff. + + The previous implementation returned ``IsStaff | IsUserInUrl``, a + composite class object that is always truthy. It therefore + unconditionally granted access regardless of the request user. + """ def has_permission(self, request, view): - return IsStaff | IsUserInUrl + return ( + IsStaff().has_permission(request, view) + or IsUserInUrl().has_permission(request, view) + ) class IsStaffOrReadOnly(permissions.BasePermission): diff --git a/openedx/core/lib/api/tests/test_permissions.py b/openedx/core/lib/api/tests/test_permissions.py index ae114522c85b..8b2649c14573 100644 --- a/openedx/core/lib/api/tests/test_permissions.py +++ b/openedx/core/lib/api/tests/test_permissions.py @@ -9,7 +9,12 @@ from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole from common.djangoapps.student.tests.factories import UserFactory -from openedx.core.lib.api.permissions import IsCourseStaffInstructor, IsMasterCourseStaffInstructor, IsStaffOrOwner +from openedx.core.lib.api.permissions import ( + IsCourseStaffInstructor, + IsMasterCourseStaffInstructor, + IsStaffOrOwner, + IsUserInUrlOrStaff, +) class TestObject: @@ -185,3 +190,60 @@ def test_has_permission_with_view_kwargs_as_non_owner(self): view = GenericAPIView() view.kwargs = {'username': UserFactory().username} assert not self.permission.has_permission(self.request, view) + + +class IsUserInUrlOrStaffTests(TestCase): + """ + Regression tests for IsUserInUrlOrStaff. + + The previous implementation returned ``IsStaff | IsUserInUrl``, a + composite class object that is always truthy. The permission + therefore unconditionally granted access to every authenticated + caller regardless of the URL subject. + """ + + def setUp(self): + super().setUp() + self.permission = IsUserInUrlOrStaff() + self.factory = RequestFactory() + self.owner = UserFactory(username='owner') + self.other = UserFactory(username='intruder') + self.staff = UserFactory(username='gremlin', is_staff=True) + + def _make_request(self, user, url_username): + request = self.factory.get(f'/api/user/v1/preferences/{url_username}/') + request.user = user + request.parser_context = {'kwargs': {'username': url_username}} + return request + + def test_sec_broken_permission_staff_granted_regression(self): + request = self._make_request(self.staff, 'owner') + assert self.permission.has_permission(request, None) is True + + def test_sec_broken_permission_user_matching_url_granted_regression(self): + request = self._make_request(self.owner, 'owner') + assert self.permission.has_permission(request, None) is True + + def test_sec_broken_permission_user_mismatch_denied_regression(self): + """ + Non-staff attempting to access another user's URL must be denied. + The regression is that previously this returned a truthy class + object (``IsStaff | IsUserInUrl``) and granted access silently. + Now the method must return a concrete ``False`` for mismatched + non-staff callers. + """ + request = self._make_request(self.other, 'owner') + assert self.permission.has_permission(request, None) is False + + def test_sec_broken_permission_anonymous_denied_regression(self): + request = self._make_request(AnonymousUser(), 'owner') + assert self.permission.has_permission(request, None) is False + + def test_sec_broken_permission_returns_boolean_not_class_regression(self): + """ + Ensure the return value is a bool, not a DRF permission class + object (which would always be truthy). + """ + request = self._make_request(self.staff, 'owner') + result = self.permission.has_permission(request, None) + assert isinstance(result, bool)