diff --git a/openedx/core/djangoapps/authz/decorators.py b/openedx/core/djangoapps/authz/decorators.py index 49b623aad368..2868da6fc65b 100644 --- a/openedx/core/djangoapps/authz/decorators.py +++ b/openedx/core/djangoapps/authz/decorators.py @@ -117,3 +117,30 @@ def get_course_key(course_id: str) -> CourseKey: # Attempt to parse it as such and extract the course key. usage_key = UsageKey.from_string(course_id) return usage_key.course_key + + +def user_has_course_permission_from_query_param( + request, + authz_permission: str, + param_name: str = "course_id", +) -> bool: + """ + Check an AuthZ course permission using a course/usage id taken from a request query param. + + Meant for endpoints that are normally scoped to a library (or another non-course resource) + but that should also grant access to a user who holds a course-level permission, e.g. a + Course Auditor reviewing a library's pending changes from within a course they can't + otherwise view the library from. The caller is expected to fall back to its regular + resource-level permission check when this returns False. + + Returns False (never raises) if the query param is absent or not a valid course/usage id, + since that just means the bypass doesn't apply, not that the request is malformed. + """ + course_id = request.GET.get(param_name) + if not course_id: + return False + try: + course_key = get_course_key(course_id) + except InvalidKeyError: + return False + return user_has_course_permission(request.user, authz_permission, course_key) diff --git a/openedx/core/djangoapps/authz/tests/test_decorators.py b/openedx/core/djangoapps/authz/tests/test_decorators.py index 68768733a435..70c22ce61dca 100644 --- a/openedx/core/djangoapps/authz/tests/test_decorators.py +++ b/openedx/core/djangoapps/authz/tests/test_decorators.py @@ -5,7 +5,11 @@ from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission -from openedx.core.djangoapps.authz.decorators import authz_permission_required, get_course_key +from openedx.core.djangoapps.authz.decorators import ( + authz_permission_required, + get_course_key, + user_has_course_permission_from_query_param, +) from openedx.core.lib.api.view_utils import DeveloperErrorResponseException @@ -154,3 +158,76 @@ def test_usage_key_string(self): result = get_course_key(str(usage_key)) self.assertEqual(result, self.course_key) # noqa: PT009 + + +class UserHasCoursePermissionFromQueryParamTests(TestCase): + """Tests for user_has_course_permission_from_query_param.""" + + def setUp(self): + self.factory = RequestFactory() + self.course_key = CourseLocator("TestX", "TST101", "2025") + self.user = Mock() + + def test_missing_param_denies_without_checking_permission(self): + """No query param at all means the bypass doesn't apply.""" + request = self.factory.get("/test") + + with patch("openedx.core.djangoapps.authz.decorators.user_has_course_permission") as mock_check: + result = user_has_course_permission_from_query_param(request, "courses.view_library_updates") + + assert result is False + mock_check.assert_not_called() + + def test_invalid_course_id_denies_without_checking_permission(self): + """A malformed course/usage id is treated as absent, not as an error.""" + request = self.factory.get("/test", {"course_id": "not-a-real-key"}) + + with patch("openedx.core.djangoapps.authz.decorators.user_has_course_permission") as mock_check: + result = user_has_course_permission_from_query_param(request, "courses.view_library_updates") + + assert result is False + mock_check.assert_not_called() + + def test_valid_course_id_delegates_to_permission_check(self): + """A valid course id is parsed and passed through to the real permission check.""" + request = self.factory.get("/test", {"course_id": str(self.course_key)}) + request.user = self.user + + with patch( + "openedx.core.djangoapps.authz.decorators.user_has_course_permission", + return_value=True, + ) as mock_check: + result = user_has_course_permission_from_query_param(request, "courses.view_library_updates") + + assert result is True + mock_check.assert_called_once_with(self.user, "courses.view_library_updates", self.course_key) + + def test_usage_key_in_param_resolves_to_its_course(self): + """A usage key (not just a bare course key) resolves to the course it belongs to.""" + usage_key = BlockUsageLocator(self.course_key, "html", "block1") + request = self.factory.get("/test", {"course_id": str(usage_key)}) + request.user = self.user + + with patch( + "openedx.core.djangoapps.authz.decorators.user_has_course_permission", + return_value=True, + ) as mock_check: + result = user_has_course_permission_from_query_param(request, "courses.view_library_updates") + + assert result is True + mock_check.assert_called_once_with(self.user, "courses.view_library_updates", self.course_key) + + def test_custom_param_name(self): + """The query param name can be overridden.""" + request = self.factory.get("/test", {"downstream_course_id": str(self.course_key)}) + request.user = self.user + + with patch( + "openedx.core.djangoapps.authz.decorators.user_has_course_permission", + return_value=True, + ): + result = user_has_course_permission_from_query_param( + request, "courses.view_library_updates", param_name="downstream_course_id", + ) + + assert result is True diff --git a/openedx/core/djangoapps/content_libraries/rest_api/containers.py b/openedx/core/djangoapps/content_libraries/rest_api/containers.py index 6a63a723ed3a..9bbf708ba611 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/containers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/containers.py @@ -19,6 +19,7 @@ from rest_framework.status import HTTP_200_OK, HTTP_204_NO_CONTENT from rest_framework.views import APIView +from openedx.core.djangoapps.authz.decorators import user_has_course_permission_from_query_param from openedx.core.djangoapps.content_libraries import api, permissions from openedx.core.lib.api.view_utils import view_auth_classes from openedx.core.types.http import RestRequest @@ -80,11 +81,14 @@ def get(self, request, container_key: LibraryContainerLocator): """ Get information about a container """ - api.require_permission_for_library_key( - container_key.lib_key, - request.user, - permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, - ) + if not user_has_course_permission_from_query_param( + request, authz_permissions.COURSES_VIEW_LIBRARY_UPDATES.identifier + ): + api.require_permission_for_library_key( + container_key.lib_key, + request.user, + permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, + ) container = api.get_container(container_key, include_collections=True) return Response(serializers.LibraryContainerMetadataSerializer(container).data) @@ -185,11 +189,14 @@ def get(self, request, container_key: LibraryContainerLocator): ] """ published = request.GET.get('published', 'false').lower() == 'true' - api.require_permission_for_library_key( - container_key.lib_key, - request.user, - permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, - ) + if not user_has_course_permission_from_query_param( + request, authz_permissions.COURSES_VIEW_LIBRARY_UPDATES.identifier + ): + api.require_permission_for_library_key( + container_key.lib_key, + request.user, + permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, + ) child_entities = api.get_container_children(container_key, published=published) if container_key.container_type == content_models.Unit.type_code: data = serializers.LibraryXBlockMetadataSerializer(child_entities, many=True).data diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 3900234f1db5..74aaefa22d5f 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -8,10 +8,16 @@ import ddt from freezegun import freeze_time from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2 +from openedx_authz.constants.roles import COURSE_AUDITOR from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.authz.tests.mixins import CourseAuthoringAuthzTestMixin from openedx.core.djangoapps.content_libraries import api -from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest +from openedx.core.djangoapps.content_libraries.tests.base import ( + URL_LIB_CONTAINER, + URL_LIB_CONTAINER_CHILDREN, + ContentLibrariesRestApiTest, +) from openedx.core.djangoapps.content_tagging import api as tagging_api from openedx.core.djangolib.testing.utils import skip_unless_cms @@ -1322,3 +1328,55 @@ def test_container_draft_history_permissions(self): unauthorized = UserFactory.create(username="noauth-container-hist", password="edx") with self.as_user(unauthorized): self._get_container_draft_history(unit["id"], expect_response=403) + + +@skip_unless_cms +class ContainerLibraryUpdatesAuthzBypassTest(CourseAuthoringAuthzTestMixin, ContentLibrariesRestApiTest): + """ + A course auditor has no direct permissions on the library backing a unit they're + reviewing, but does hold `courses.view_library_updates` in the course. Passing that + course as `course_id` should let them view the container/children anyway. + + See openedx-authz#441. + """ + + def setUp(self): + super().setUp() + self.course_id = "course-v1:CL-TEST+TST101+2025" + self.add_user_to_role_in_course(self.authorized_user, COURSE_AUDITOR.external_key, self.course_id) + + self.lib = self._create_library(slug="library-updates-lib", title="Library Updates Test Library") + self.unit = self._create_container(self.lib["id"], "unit", display_name="Reviewable Unit", slug=None) + + def test_container_detail_denied_without_course_id(self): + with self.as_user(self.authorized_user): + response = self.client.get(URL_LIB_CONTAINER.format(container_key=self.unit["id"])) + assert response.status_code == 403 + + def test_container_detail_allowed_with_course_id(self): + with self.as_user(self.authorized_user): + response = self.client.get( + URL_LIB_CONTAINER.format(container_key=self.unit["id"]), {"course_id": self.course_id}, + ) + assert response.status_code == 200 + + def test_container_children_denied_without_course_id(self): + with self.as_user(self.authorized_user): + response = self.client.get(URL_LIB_CONTAINER_CHILDREN.format(container_key=self.unit["id"])) + assert response.status_code == 403 + + def test_container_children_allowed_with_course_id(self): + with self.as_user(self.authorized_user): + response = self.client.get( + URL_LIB_CONTAINER_CHILDREN.format(container_key=self.unit["id"]), {"course_id": self.course_id}, + ) + assert response.status_code == 200 + + def test_unrelated_course_id_is_denied(self): + """A course_id where the user holds no role at all must not grant access.""" + with self.as_user(self.authorized_user): + response = self.client.get( + URL_LIB_CONTAINER.format(container_key=self.unit["id"]), + {"course_id": "course-v1:CL-TEST+OTHER101+2025"}, + ) + assert response.status_code == 403 diff --git a/openedx/core/djangoapps/content_libraries/tests/test_embed_block.py b/openedx/core/djangoapps/content_libraries/tests/test_embed_block.py index 8f0fb85eebc8..9e7f258a21f1 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_embed_block.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_embed_block.py @@ -9,9 +9,11 @@ import pytest from django.core.exceptions import ValidationError from django.test.utils import override_settings +from openedx_authz.constants.roles import COURSE_AUDITOR from xblock.core import XBlock -from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest +from openedx.core.djangoapps.authz.tests.mixins import CourseAuthoringAuthzTestMixin +from openedx.core.djangoapps.content_libraries.tests.base import URL_BLOCK_EMBED_VIEW, ContentLibrariesRestApiTest from openedx.core.djangolib.testing.utils import skip_unless_cms from .fields_test_block import FieldsTestBlock @@ -227,3 +229,48 @@ def test_embed_view_versions_static_assets(self): # TODO: if we are ever able to run these tests in the LMS, test that the LMS only allows accessing the published # version. + + +@skip_unless_cms +@override_settings(CORS_ORIGIN_WHITELIST=[]) # For some reason, this setting isn't defined in our test environment? +class EmbedViewAuthzBypassTest(CourseAuthoringAuthzTestMixin, ContentLibrariesRestApiTest): + """ + A course auditor has no direct permissions on the library backing a block they're + reviewing, but does hold `courses.view_library_updates` in the course. Passing that + course as `course_id` should let them view the block's embed anyway. + + See openedx-authz#441. + """ + + def setUp(self): + super().setUp() + self.course_id = "course-v1:CL-TEST+TST101+2025" + self.add_user_to_role_in_course(self.authorized_user, COURSE_AUDITOR.external_key, self.course_id) + + lib = self._create_library(slug="embed-authz-bypass-lib", title="Embed AuthZ Bypass Test Library") + create_response = self._add_block_to_library(lib["id"], "html", "block1") + self.block_id = create_response["id"] + self._set_library_block_olx(self.block_id, "Hello world") + self._commit_library_changes(lib["id"]) + + def test_embed_denied_without_course_id(self): + with self.as_user(self.authorized_user): + response = self.client.get(URL_BLOCK_EMBED_VIEW.format(block_key=self.block_id, view_name="student_view")) + assert response.status_code == 403 + + def test_embed_allowed_with_course_id(self): + with self.as_user(self.authorized_user): + response = self.client.get( + URL_BLOCK_EMBED_VIEW.format(block_key=self.block_id, view_name="student_view"), + {"course_id": self.course_id}, + ) + assert response.status_code == 200 + + def test_unrelated_course_id_is_denied(self): + """A course_id where the user holds no role at all must not grant access.""" + with self.as_user(self.authorized_user): + response = self.client.get( + URL_BLOCK_EMBED_VIEW.format(block_key=self.block_id, view_name="student_view"), + {"course_id": "course-v1:CL-TEST+OTHER101+2025"}, + ) + assert response.status_code == 403 diff --git a/openedx/core/djangoapps/xblock/rest_api/views.py b/openedx/core/djangoapps/xblock/rest_api/views.py index 36fc1b9ae6d1..403c99a89a7c 100644 --- a/openedx/core/djangoapps/xblock/rest_api/views.py +++ b/openedx/core/djangoapps/xblock/rest_api/views.py @@ -14,6 +14,7 @@ from django.views.decorators.clickjacking import xframe_options_exempt from django.views.decorators.csrf import csrf_exempt from opaque_keys.edx.keys import UsageKeyV2 +from openedx_authz.constants.permissions import COURSES_VIEW_LIBRARY_UPDATES from rest_framework import permissions, serializers from rest_framework.decorators import api_view, permission_classes # pylint: disable=unused-import from rest_framework.exceptions import AuthenticationFailed, NotFound, PermissionDenied @@ -26,6 +27,7 @@ import openedx.core.djangoapps.site_configuration.helpers as configuration_helpers from common.djangoapps.util.json_request import JsonResponse +from openedx.core.djangoapps.authz.decorators import user_has_course_permission_from_query_param from openedx.core.djangoapps.xblock.learning_context.manager import get_learning_context_impl from openedx.core.lib.api.view_utils import view_auth_classes @@ -104,8 +106,18 @@ def embed_block_view(request, usage_key: UsageKeyV2, view_name: str): except ValueError as exc: raise serializers.ValidationError("Invalid version specifier") from exc + # A user reviewing a library's pending changes from within a course (e.g. a Course + # Auditor with courses.view_library_updates) may not have direct access to the + # upstream library. That course-level permission substitutes for the regular + # library-level check below. + check_permission: CheckPerm | None = CheckPerm.CAN_LEARN + if user_has_course_permission_from_query_param( + request, COURSES_VIEW_LIBRARY_UPDATES.identifier + ): + check_permission = None + try: - block = load_block(usage_key, request.user, check_permission=CheckPerm.CAN_LEARN, version=version) + block = load_block(usage_key, request.user, check_permission=check_permission, version=version) except NoSuchUsage as exc: raise NotFound(f"{usage_key} not found") from exc