From a221c031446f545b43f9eb32368244286448b435 Mon Sep 17 00:00:00 2001 From: Serhiii Nanai <59769569+Serj-N@users.noreply.github.com> Date: Thu, 30 Oct 2025 23:23:49 +0200 Subject: [PATCH 01/21] feat: [FC-0092] Optimize Course Info Blocks API (#37122) The Course Info Blocks API endpoint has been known to be rather slow to return the response. Previous investigation showed that the major time sink was the get_course_blocks function, which is called three times in a single request. This commit aims to improve the response times by reducing the number of times that this function is called. Solution Summary The first time the function get_course_blocks is called, the result (transformed course blocks) is stored in the current WSGI request object. Later in the same request, before the second get_course_blocks call is triggered, the already transformed course blocks are taken from the request object, and if they are available, get_course_blocks is not called (if not, it is called as a fallback). Later in the request, the function is called again as before (see Optimization Strategy and Difficulties). Optimization Strategy and Difficulties The original idea was to fetch and transform the course blocks once and reuse them in all three cases, which would reduce get_course_blocks call count to 1. However, this did not turn out to be a viable solution because of the arguments passed to get_course_blocks. Notably, the allow_start_dates_in_future boolean flag affects the behavior of StartDateTransformer, which is a filtering transformer modifying the block structure returned. The first two times allow_start_dates_in_future is False, the third time it is True. Setting it to True in all three cases would mean that some blocks would be incorrectly included in the response. This left us with one option - optimize the first two calls. The difference between the first two calls is the non-filtering transformers, however the second call applies a subset of transformers from the first call, so it was safe to apply the superset of transformers in both cases. This allowed to reduce the number of function calls to 2. However, the cached structure may be further mutated by filters downstream, which means we need to cache a copy of the course structure (not the structure itself). The copy method itself is quite heavy (it calls deepcopy three times), making the benefits of this solution much less tangible. In fact, another potential optimization that was considered was to reuse the collected block structure (pre-transformation), but since calling copy on a collected structure proved to be more time-consuming than calling get_collected, this change was discarded, considering that the goal is to improve performance. Revised Solution To achieve a more tangible performance improvement, it was decided to modify the previous strategy as follows: * Pass a for_blocks_view parameter to the get_blocks function to make sure the new caching logic only affects the blocks view. * Collect and cache course blocks with future dates included. * Include start key in requested fields. * Reuse the cached blocks in the third call, which is in get_course_assignments * Before returning the response, filter out any blocks with a future start date, and also remove the start key if it was not in requested fields --- lms/djangoapps/course_api/blocks/api.py | 24 ++++++- lms/djangoapps/course_api/blocks/utils.py | 20 ++++++ lms/djangoapps/course_api/blocks/views.py | 43 +++++++++++ lms/djangoapps/courseware/courses.py | 6 +- lms/djangoapps/grades/course_data.py | 8 ++- .../tests/test_course_info_views.py | 72 +++++++++++++++++++ 6 files changed, 168 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/course_api/blocks/api.py b/lms/djangoapps/course_api/blocks/api.py index a79e9759e191..80c2bb7fc5ff 100644 --- a/lms/djangoapps/course_api/blocks/api.py +++ b/lms/djangoapps/course_api/blocks/api.py @@ -1,7 +1,7 @@ """ API function for retrieving course blocks data """ - +from edx_django_utils.cache import RequestCache import lms.djangoapps.course_blocks.api as course_blocks_api from lms.djangoapps.course_blocks.transformers.access_denied_filter import AccessDeniedMessageFilterTransformer @@ -14,6 +14,7 @@ from .toggles import HIDE_ACCESS_DENIALS_FLAG from .transformers.blocks_api import BlocksAPITransformer from .transformers.milestones import MilestonesAndSpecialExamsTransformer +from .utils import COURSE_API_REQUEST_CACHE_NAMESPACE, REUSABLE_BLOCKS_CACHE_KEY def get_blocks( @@ -29,6 +30,7 @@ def get_blocks( block_types_filter=None, hide_access_denials=False, allow_start_dates_in_future=False, + cache_with_future_dates=False, ): """ Return a serialized representation of the course blocks. @@ -61,6 +63,7 @@ def get_blocks( allow_start_dates_in_future (bool): When True, will allow blocks to be returned that can bypass the StartDateTransformer's filter to show blocks with start dates in the future. + cache_with_future_dates (bool): When True, will use the block caching logic using RequestCache """ if HIDE_ACCESS_DENIALS_FLAG.is_enabled(): @@ -118,6 +121,10 @@ def get_blocks( ), ] + if cache_with_future_dates: + # Include future dates such that get_course_assignments can reuse the block structure from RequestCache + allow_start_dates_in_future = True + # transform blocks = course_blocks_api.get_course_blocks( user, @@ -128,6 +135,19 @@ def get_blocks( include_has_scheduled_content=include_has_scheduled_content ) + if cache_with_future_dates: + # Store a copy of the transformed, but still unfiltered, course blocks in RequestCache to be reused + # wherever possible for optimization. Copying is required to make sure the cached structure is not mutated + # by the filtering below. + request_cache = RequestCache(COURSE_API_REQUEST_CACHE_NAMESPACE) + request_cache.set(REUSABLE_BLOCKS_CACHE_KEY, blocks.copy()) + + # Since we included blocks with future start dates in our block structure, + # we need to include the 'start' field to filter out such blocks before returning the response. + # If 'start' field is not requested, it will be removed from the response. + requested_fields = set(requested_fields) + requested_fields.add('start') + # filter blocks by types if block_types_filter: block_keys_to_remove = [] @@ -142,7 +162,7 @@ def get_blocks( serializer_context = { 'request': request, 'block_structure': blocks, - 'requested_fields': requested_fields or [], + 'requested_fields': requested_fields, } if return_type == 'dict': diff --git a/lms/djangoapps/course_api/blocks/utils.py b/lms/djangoapps/course_api/blocks/utils.py index 6f371624b7df..0686abc2fac1 100644 --- a/lms/djangoapps/course_api/blocks/utils.py +++ b/lms/djangoapps/course_api/blocks/utils.py @@ -1,6 +1,7 @@ """ Utils for Blocks """ +from edx_django_utils.cache import RequestCache from rest_framework.utils.serializer_helpers import ReturnList from openedx.core.djangoapps.discussions.models import ( @@ -9,6 +10,10 @@ ) +COURSE_API_REQUEST_CACHE_NAMESPACE = "course_api" +REUSABLE_BLOCKS_CACHE_KEY = "reusable_transformed_blocks" + + def filter_discussion_xblocks_from_response(response, course_key): """ Removes discussion xblocks if discussion provider is openedx. @@ -63,3 +68,18 @@ def filter_discussion_xblocks_from_response(response, course_key): response.data['blocks'] = filtered_blocks return response + + +def get_cached_transformed_blocks(): + """ + Helper function to get an unfiltered course structure from RequestCache, + including blocks with start dates in the future. + + Caution: For performance reasons, the function returns the structure object itself, not its copy. + This means the retrieved structure is supposed to be read-only and should not be mutated by consumers. + """ + request_cache = RequestCache(COURSE_API_REQUEST_CACHE_NAMESPACE) + cached_response = request_cache.get_cached_response(REUSABLE_BLOCKS_CACHE_KEY) + reusable_transformed_blocks = cached_response.value if cached_response.is_found else None + + return reusable_transformed_blocks diff --git a/lms/djangoapps/course_api/blocks/views.py b/lms/djangoapps/course_api/blocks/views.py index 96679a562957..7f81861b9b7b 100644 --- a/lms/djangoapps/course_api/blocks/views.py +++ b/lms/djangoapps/course_api/blocks/views.py @@ -2,6 +2,7 @@ CourseBlocks API views """ +from datetime import datetime, timezone from django.core.exceptions import ValidationError from django.db import transaction @@ -237,6 +238,7 @@ def list(self, request, usage_key_string, hide_access_denials=False): # pylint: params.cleaned_data['return_type'], params.cleaned_data.get('block_types_filter', None), hide_access_denials=hide_access_denials, + cache_with_future_dates=True ) ) # If the username is an empty string, and not None, then we are requesting @@ -339,9 +341,50 @@ def list(self, request, hide_access_denials=False): # pylint: disable=arguments if not root: raise ValidationError(f"Unable to find course block in '{course_key_string}'") + # Earlier we included blocks with future start dates in the collected/cached block structure. + # Now we need to emulate allow_start_dates_in_future=False by removing any such blocks. + include_start = "start" in request.query_params['requested_fields'] + self.remove_future_blocks(course_blocks, include_start) + recurse_mark_complete(root, course_blocks) return response + @staticmethod + def remove_future_blocks(course_blocks, include_start: bool): + """ + Mutates course_blocks in place: + - removes blocks whose 'start' is in the future + - also removes references to them from parents' 'children' lists + - removes 'start' key from all blocks if it wasn't requested + """ + if not course_blocks: + return course_blocks + + now = datetime.now(timezone.utc) + + # 1. Collect IDs of blocks to remove + to_remove = set() + for block_id, block in course_blocks.items(): + get_field = block.get if include_start else block.pop + start = get_field("start") + if start and start > now: + to_remove.add(block_id) + + if not to_remove: + return course_blocks + + # 2. Remove the blocks themselves + for block_id in to_remove: + course_blocks.pop(block_id, None) + + # 3. Clean up children lists + for block in course_blocks.values(): + children = block.get("children") + if children: + block["children"] = [cid for cid in children if cid not in to_remove] + + return course_blocks + @method_decorator(transaction.non_atomic_requests, name='dispatch') @view_auth_classes(is_authenticated=False) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index bbf9d5394705..3b527cda4dfa 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -26,6 +26,7 @@ from common.djangoapps.static_replace import replace_static_urls from common.djangoapps.util.date_utils import strftime_localized from lms.djangoapps import branding +from lms.djangoapps.course_api.blocks.utils import get_cached_transformed_blocks from lms.djangoapps.course_blocks.api import get_course_blocks from lms.djangoapps.courseware.access import has_access from lms.djangoapps.courseware.access_response import ( @@ -636,7 +637,10 @@ def get_course_assignments(course_key, user, include_access=False, include_witho store = modulestore() course_usage_key = store.make_course_usage_key(course_key) - block_data = get_course_blocks(user, course_usage_key, allow_start_dates_in_future=True, include_completion=True) + + block_data = get_cached_transformed_blocks() or get_course_blocks( + user, course_usage_key, allow_start_dates_in_future=True, include_completion=True + ) now = datetime.now(pytz.UTC) assignments = [] diff --git a/lms/djangoapps/grades/course_data.py b/lms/djangoapps/grades/course_data.py index 5464c4f88105..523d6e6df38d 100644 --- a/lms/djangoapps/grades/course_data.py +++ b/lms/djangoapps/grades/course_data.py @@ -2,12 +2,12 @@ Code used to get and cache the requested course-data """ - from lms.djangoapps.course_blocks.api import get_course_blocks from openedx.core.djangoapps.content.block_structure.api import get_block_structure_manager from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from .transformer import GradesTransformer +from ..course_api.blocks.utils import get_cached_transformed_blocks class CourseData: @@ -56,7 +56,11 @@ def location(self): # lint-amnesty, pylint: disable=missing-function-docstring @property def structure(self): # lint-amnesty, pylint: disable=missing-function-docstring if self._structure is None: - self._structure = get_course_blocks( + # The get_course_blocks function proved to be a major time sink during a request at "blocks/". + # This caching logic helps improve the response time by getting a copy of the already transformed, but still + # unfiltered, course blocks from RequestCache and thus reducing the number of times that + # the get_course_blocks function is called. + self._structure = get_cached_transformed_blocks() or get_course_blocks( self.user, self.location, collected_block_structure=self._collected_block_structure, diff --git a/lms/djangoapps/mobile_api/tests/test_course_info_views.py b/lms/djangoapps/mobile_api/tests/test_course_info_views.py index efb3f7d9fdbb..b7cdc6b03961 100644 --- a/lms/djangoapps/mobile_api/tests/test_course_info_views.py +++ b/lms/djangoapps/mobile_api/tests/test_course_info_views.py @@ -432,6 +432,78 @@ def test_extend_sequential_info_with_assignment_progress_for_other_types(self, b for block_info in response.data['blocks'].values(): self.assertNotEqual('assignment_progress', block_info) + def test_response_keys(self): + response = self.verify_response(url=self.url) + data = response.data + + expected_top_level_keys = { + 'blocks', + 'certificate', + 'course_about', + 'course_access_details', + 'course_handouts', + 'course_modes', + 'course_progress', + 'course_sharing_utm_parameters', + 'course_updates', + 'deprecate_youtube', + 'discussion_url', + 'end', + 'enrollment_details', + 'id', + 'is_self_paced', + 'media', + 'name', + 'number', + 'org', + 'org_logo', + 'root', + 'start', + 'start_display', + 'start_type' + } + expected_course_access_keys = { + "has_unmet_prerequisites", + "is_too_early", + "is_staff", + "audit_access_expires", + "courseware_access" + } + expected_courseware_access_keys = { + "has_access", + "error_code", + "developer_message", + "user_message", + "additional_context_user_message", + "user_fragment" + } + expected_enrollment_details_keys = {"created", "mode", "is_active", "upgrade_deadline"} + expected_media_keys = {"image"} + expected_image_keys = {"raw", "small", "large"} + expected_course_sharing_keys = {"facebook", "twitter"} + expected_course_modes_keys = {"slug", "sku", "android_sku", "ios_sku", "min_price"} + expected_course_progress_keys = {"total_assignments_count", "assignments_completed"} + + self.assertSetEqual(set(data), expected_top_level_keys) + self.assertSetEqual(set(data["course_access_details"]), expected_course_access_keys) + self.assertSetEqual(set(data["course_access_details"]["courseware_access"]), expected_courseware_access_keys) + self.assertSetEqual(set(data["enrollment_details"]), expected_enrollment_details_keys) + self.assertSetEqual(set(data["media"]), expected_media_keys) + self.assertSetEqual(set(data["media"]["image"]), expected_image_keys) + self.assertSetEqual(set(data["course_sharing_utm_parameters"]), expected_course_sharing_keys) + self.assertSetEqual(set(data["course_modes"][0]), expected_course_modes_keys) + self.assertSetEqual(set(data["course_progress"]), expected_course_progress_keys) + + def test_block_count_depends_on_depth_in_request_params(self): + response_depth_zero = self.verify_response(url=self.url, params={'depth': 0}) + response_depth_one = self.verify_response(url=self.url, params={'depth': 1}) + blocks_depth_zero = [block for block in self.store.get_items(self.course_key) if block.category == "course"] + blocks_depth_one = [ + block for block in self.store.get_items(self.course_key) if block.category in ("course", "chapter") + ] + self.assertEqual(len(response_depth_zero.data["blocks"]), len(blocks_depth_zero)) + self.assertEqual(len(response_depth_one.data["blocks"]), len(blocks_depth_one)) + class TestCourseEnrollmentDetailsView(MobileAPITestCase, MilestonesTestCaseMixin): # lint-amnesty, pylint: disable=test-inherits-tests """ From b48c4afd0ec0fe3f8a0c2656ea418f572c356ada Mon Sep 17 00:00:00 2001 From: Peter Pinch Date: Mon, 3 Nov 2025 09:22:26 -0500 Subject: [PATCH 02/21] Merge pull request #37569 from mitodl/arslan/fix-validation-api fix: validation API for certificates --- .../contentstore/api/tests/test_validation.py | 94 +++++++++++-------- .../contentstore/views/certificate_manager.py | 2 +- 2 files changed, 58 insertions(+), 38 deletions(-) diff --git a/cms/djangoapps/contentstore/api/tests/test_validation.py b/cms/djangoapps/contentstore/api/tests/test_validation.py index 4e0a9bbce666..4928d31dc1f2 100644 --- a/cms/djangoapps/contentstore/api/tests/test_validation.py +++ b/cms/djangoapps/contentstore/api/tests/test_validation.py @@ -2,9 +2,11 @@ Tests for the course import API views """ - +import factory from datetime import datetime +from django.conf import settings +import ddt from django.test.utils import override_settings from django.urls import reverse from rest_framework import status @@ -12,10 +14,13 @@ from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory +from common.djangoapps.course_modes.models import CourseMode +from common.djangoapps.course_modes.tests.factories import CourseModeFactory from common.djangoapps.student.tests.factories import StaffFactory from common.djangoapps.student.tests.factories import UserFactory +@ddt.ddt @override_settings(PROCTORING_BACKENDS={'DEFAULT': 'proctortrack', 'proctortrack': {}}) class CourseValidationViewTest(SharedModuleStoreTestCase, APITestCase): """ @@ -82,39 +87,54 @@ def test_student_fails(self): resp = self.client.get(self.get_url(self.course_key)) self.assertEqual(resp.status_code, status.HTTP_403_FORBIDDEN) - def test_staff_succeeds(self): - self.client.login(username=self.staff.username, password=self.password) - resp = self.client.get(self.get_url(self.course_key), {'all': 'true'}) - self.assertEqual(resp.status_code, status.HTTP_200_OK) - expected_data = { - 'assignments': { - 'total_number': 1, - 'total_visible': 1, - 'assignments_with_dates_before_start': [], - 'assignments_with_dates_after_end': [], - 'assignments_with_ora_dates_after_end': [], - 'assignments_with_ora_dates_before_start': [], - }, - 'dates': { - 'has_start_date': True, - 'has_end_date': False, - }, - 'updates': { - 'has_update': True, - }, - 'certificates': { - 'is_enabled': False, - 'is_activated': False, - 'has_certificate': False, - }, - 'grades': { - 'has_grading_policy': False, - 'sum_of_weights': 1.0, - }, - 'proctoring': { - 'needs_proctoring_escalation_email': True, - 'has_proctoring_escalation_email': True, - }, - 'is_self_paced': True, - } - self.assertDictEqual(resp.data, expected_data) + @ddt.data( + (False, False), + (True, False), + (False, True), + (True, True), + ) + @ddt.unpack + def test_staff_succeeds(self, certs_html_view, with_modes): + features = dict(settings.FEATURES, CERTIFICATES_HTML_VIEW=certs_html_view) + with override_settings(FEATURES=features): + if with_modes: + CourseModeFactory.create_batch( + 2, + course_id=self.course.id, + mode_slug=factory.Iterator([CourseMode.AUDIT, CourseMode.VERIFIED]), + ) + self.client.login(username=self.staff.username, password=self.password) + resp = self.client.get(self.get_url(self.course_key), {'all': 'true'}) + self.assertEqual(resp.status_code, status.HTTP_200_OK) + expected_data = { + 'assignments': { + 'total_number': 1, + 'total_visible': 1, + 'assignments_with_dates_before_start': [], + 'assignments_with_dates_after_end': [], + 'assignments_with_ora_dates_after_end': [], + 'assignments_with_ora_dates_before_start': [], + }, + 'dates': { + 'has_start_date': True, + 'has_end_date': False, + }, + 'updates': { + 'has_update': True, + }, + 'certificates': { + 'is_enabled': with_modes, + 'is_activated': False, + 'has_certificate': False, + }, + 'grades': { + 'has_grading_policy': False, + 'sum_of_weights': 1.0, + }, + 'proctoring': { + 'needs_proctoring_escalation_email': True, + 'has_proctoring_escalation_email': True, + }, + 'is_self_paced': True, + } + self.assertDictEqual(resp.data, expected_data) diff --git a/cms/djangoapps/contentstore/views/certificate_manager.py b/cms/djangoapps/contentstore/views/certificate_manager.py index 429950477fdd..081afdcc0dd7 100644 --- a/cms/djangoapps/contentstore/views/certificate_manager.py +++ b/cms/djangoapps/contentstore/views/certificate_manager.py @@ -121,7 +121,7 @@ def is_activated(course): along with the certificates. """ is_active = False - certificates = None + certificates = [] if settings.FEATURES.get('CERTIFICATES_HTML_VIEW', False): certificates = CertificateManager.get_certificates(course) # we are assuming only one certificate in certificates collection. From 93f361e017299a4c0ad66b170d331c3d23405561 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Thu, 6 Nov 2025 04:42:12 +0530 Subject: [PATCH 03/21] fix: mark container as ready to sync if any child block is deleted (#37606) Backport of https://github.com/openedx/edx-platform/pull/37603 --- cms/djangoapps/contentstore/models.py | 19 ++- .../v2/views/tests/test_downstreams.py | 113 +++++++++++++++++- cms/lib/xblock/upstream_sync.py | 9 +- 3 files changed, 134 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/models.py b/cms/djangoapps/contentstore/models.py index f5ee218f3e11..a4f2ce3c6119 100644 --- a/cms/djangoapps/contentstore/models.py +++ b/cms/djangoapps/contentstore/models.py @@ -7,12 +7,12 @@ from config_models.models import ConfigurationModel from django.db import models -from django.db.models import QuerySet, OuterRef, Case, When, Exists, Value, ExpressionWrapper -from django.db.models.fields import IntegerField, TextField, BooleanField +from django.db.models import Case, Exists, ExpressionWrapper, OuterRef, Q, QuerySet, Value, When +from django.db.models.fields import BooleanField, IntegerField, TextField from django.db.models.functions import Coalesce from django.db.models.lookups import GreaterThan from django.utils.translation import gettext_lazy as _ -from opaque_keys.edx.django.models import CourseKeyField, ContainerKeyField, UsageKeyField +from opaque_keys.edx.django.models import ContainerKeyField, CourseKeyField, UsageKeyField from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys.edx.locator import LibraryContainerLocator from openedx_learning.api.authoring import get_published_version @@ -23,7 +23,6 @@ manual_date_time_field, ) - logger = logging.getLogger(__name__) @@ -391,7 +390,7 @@ def filter_links( cls.objects.filter(**link_filter).select_related(*RELATED_FIELDS), ) if ready_to_sync is not None: - result = result.filter(ready_to_sync=ready_to_sync) + result = result.filter(Q(ready_to_sync=ready_to_sync) | Q(ready_to_sync_from_children=ready_to_sync)) # Handle top-level parents logic if use_top_level_parents: @@ -436,6 +435,11 @@ def _annotate_query_with_ready_to_sync(cls, query_set: QuerySet["EntityLinkBase" ), then=1 ), + # If upstream block was deleted, set ready_to_sync = True + When( + Q(upstream_container__publishable_entity__published__version__version_num__isnull=True), + then=1 + ), default=0, output_field=models.IntegerField() ) @@ -457,6 +461,11 @@ def _annotate_query_with_ready_to_sync(cls, query_set: QuerySet["EntityLinkBase" ), then=1 ), + # If upstream block was deleted, set ready_to_sync = True + When( + Q(upstream_block__publishable_entity__published__version__version_num__isnull=True), + then=1 + ), default=0, output_field=models.IntegerField() ) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py index ad10e373cfc8..b33d980732fa 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_downstreams.py @@ -23,7 +23,7 @@ from common.djangoapps.student.tests.factories import UserFactory from openedx.core.djangoapps.content_libraries import api as lib_api from xmodule.modulestore.django import modulestore -from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase, ImmediateOnCommitMixin +from xmodule.modulestore.tests.django_utils import ImmediateOnCommitMixin, SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory from .. import downstreams as downstreams_views @@ -32,6 +32,7 @@ URL_PREFIX = '/api/libraries/v2/' URL_LIB_CREATE = URL_PREFIX URL_LIB_BLOCKS = URL_PREFIX + '{lib_key}/blocks/' +URL_LIB_BLOCK = URL_PREFIX + 'blocks/{block_key}/' URL_LIB_BLOCK_PUBLISH = URL_PREFIX + 'blocks/{block_key}/publish/' URL_LIB_BLOCK_OLX = URL_PREFIX + 'blocks/{block_key}/olx/' URL_LIB_CONTAINER = URL_PREFIX + 'containers/{container_key}/' # Get a container in this library @@ -277,6 +278,10 @@ def _create_container(self, lib_key, container_type, slug: str | None, display_n data["slug"] = slug return self._api('post', URL_LIB_CONTAINERS.format(lib_key=lib_key), data, expect_response) + def _delete_component(self, block_key, expect_response=200): + """ Publish all changes in the specified container + children """ + return self._api('delete', URL_LIB_BLOCK.format(block_key=block_key), None, expect_response) + class SharedErrorTestCases(_BaseDownstreamViewTestMixin): """ @@ -1503,3 +1508,109 @@ def test_200_summary(self): 'last_published_at': self.now.strftime('%Y-%m-%dT%H:%M:%S.%fZ'), }] self.assertListEqual(data, expected) + + +class GetDownstreamDeletedUpstream( + _BaseDownstreamViewTestMixin, + ImmediateOnCommitMixin, + SharedModuleStoreTestCase, +): + """ + Test that parent container is marked ready_to_sync when even when the only change is a deleted component under it + """ + def call_api( + self, + course_id: str | None = None, + ready_to_sync: bool | None = None, + upstream_key: str | None = None, + item_type: str | None = None, + use_top_level_parents: bool | None = None, + ): + data = {} + if course_id is not None: + data["course_id"] = str(course_id) + if ready_to_sync is not None: + data["ready_to_sync"] = str(ready_to_sync) + if upstream_key is not None: + data["upstream_key"] = str(upstream_key) + if item_type is not None: + data["item_type"] = str(item_type) + if use_top_level_parents is not None: + data["use_top_level_parents"] = str(use_top_level_parents) + return self.client.get("/api/contentstore/v2/downstreams/", data=data) + + def test_delete_component_should_be_ready_to_sync(self): + """ + Test deleting a component from library should mark the entire section container ready to sync + """ + # Create blocks + section_id = self._create_container(self.library_id, "section", "section-12", "Section 12")["id"] + subsection_id = self._create_container(self.library_id, "subsection", "subsection-12", "Subsection 12")["id"] + unit_id = self._create_container(self.library_id, "unit", "unit-12", "Unit 12")["id"] + video_id = self._add_block_to_library(self.library_id, "video", "video-bar-13")["id"] + section_key = ContainerKey.from_string(section_id) + subsection_key = ContainerKey.from_string(subsection_id) + unit_key = ContainerKey.from_string(unit_id) + video_key = LibraryUsageLocatorV2.from_string(video_id) + + # Set children + lib_api.update_container_children(section_key, [subsection_key], None) + lib_api.update_container_children(subsection_key, [unit_key], None) + lib_api.update_container_children(unit_key, [video_key], None) + self._publish_container(unit_id) + self._publish_container(subsection_id) + self._publish_container(section_id) + self._publish_library_block(video_id) + course = CourseFactory.create(display_name="Course New") + add_users(self.superuser, CourseStaffRole(course.id), self.course_user) + chapter = BlockFactory.create( + category='chapter', parent=course, upstream=section_id, upstream_version=2, + ) + sequential = BlockFactory.create( + category='sequential', + parent=chapter, + upstream=subsection_id, + upstream_version=2, + top_level_downstream_parent_key=get_block_key_string(chapter.usage_key), + ) + vertical = BlockFactory.create( + category='vertical', + parent=sequential, + upstream=unit_id, + upstream_version=2, + top_level_downstream_parent_key=get_block_key_string(chapter.usage_key), + ) + BlockFactory.create( + category='video', + parent=vertical, + upstream=video_id, + upstream_version=1, + top_level_downstream_parent_key=get_block_key_string(chapter.usage_key), + ) + self._delete_component(video_id) + self._publish_container(unit_id) + response = self.call_api(course_id=course.id, ready_to_sync=True, use_top_level_parents=True) + assert response.status_code == 200 + data = response.json()['results'] + assert len(data) == 1 + date_format = self.now.isoformat().split("+")[0] + 'Z' + expected_results = { + 'created': date_format, + 'downstream_context_key': str(course.id), + 'downstream_usage_key': str(chapter.usage_key), + 'downstream_customized': [], + 'id': 8, + 'ready_to_sync': False, + 'ready_to_sync_from_children': True, + 'top_level_parent_usage_key': None, + 'updated': date_format, + 'upstream_context_key': self.library_id, + 'upstream_context_title': self.library_title, + 'upstream_key': section_id, + 'upstream_type': 'container', + 'upstream_version': 2, + 'version_declined': None, + 'version_synced': 2, + } + + self.assertDictEqual(data[0], expected_results) diff --git a/cms/lib/xblock/upstream_sync.py b/cms/lib/xblock/upstream_sync.py index 8a089aeda75c..b56e0d95684d 100644 --- a/cms/lib/xblock/upstream_sync.py +++ b/cms/lib/xblock/upstream_sync.py @@ -87,6 +87,13 @@ class UpstreamLink: downstream_customized: list[str] | None # List of fields modified in downstream has_top_level_parent: bool # True if this Upstream link has a top-level parent + @property + def is_upstream_deleted(self) -> bool: + return bool( + self.upstream_ref and + self.version_available is None + ) + @property def is_ready_to_sync_individually(self) -> bool: return bool( @@ -94,7 +101,7 @@ def is_ready_to_sync_individually(self) -> bool: self.version_available and self.version_available > (self.version_synced or 0) and self.version_available > (self.version_declined or 0) - ) + ) or self.is_upstream_deleted def _check_children_ready_to_sync(self, xblock_downstream: XBlock, return_fast: bool) -> list[dict[str, str]]: """ From 72c23ac570e6eda3d8077eed4bdf7cb72ef6b383 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Fri, 7 Nov 2025 12:07:17 -0500 Subject: [PATCH 04/21] fix: bump learning-core to 0.30.0 (#37615) This pulls in publishing dependency changes from: https://github.com/openedx/openedx-learning/pull/369 This fixes a bug where publishing a Content Library v2 container would publish only its direct children instead of publishing all ancestors. Backports: 190a8b8160352d3c2eba9167226fa05f9589188d Co-authored-by: Kyle McCormick --- .../djangoapps/content_libraries/tests/test_containers.py | 6 +++--- requirements/common_constraints.txt | 2 +- requirements/constraints.txt | 2 +- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 8b7b0c527381..7e6eac3beda8 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -630,7 +630,7 @@ def test_section_hierarchy(self): ] def test_subsection_hierarchy(self): - with self.assertNumQueries(93): + with self.assertNumQueries(95): hierarchy = self._get_container_hierarchy(self.subsection_with_units["id"]) assert hierarchy["object_key"] == self.subsection_with_units["id"] assert hierarchy["components"] == [ @@ -653,7 +653,7 @@ def test_subsection_hierarchy(self): ] def test_units_hierarchy(self): - with self.assertNumQueries(56): + with self.assertNumQueries(60): hierarchy = self._get_container_hierarchy(self.unit_with_components["id"]) assert hierarchy["object_key"] == self.unit_with_components["id"] assert hierarchy["components"] == [ @@ -679,7 +679,7 @@ def test_container_hierarchy_not_found(self): ) def test_block_hierarchy(self): - with self.assertNumQueries(21): + with self.assertNumQueries(27): hierarchy = self._get_block_hierarchy(self.problem_block["id"]) assert hierarchy["object_key"] == self.problem_block["id"] assert hierarchy["components"] == [ diff --git a/requirements/common_constraints.txt b/requirements/common_constraints.txt index 28ebe29f5cc9..c13c406e6176 100644 --- a/requirements/common_constraints.txt +++ b/requirements/common_constraints.txt @@ -23,7 +23,7 @@ # See https://github.com/openedx/edx-platform/issues/35126 for more info elasticsearch<7.14.0 -# pip 25.3 is incompatible with pip-tools hence causing failures during the build process +# pip 25.3 is incompatible with pip-tools hence causing failures during the build process # Make upgrade command and all requirements upgrade jobs are broken due to this. # See issue https://github.com/openedx/public-engineering/issues/440 for details regarding the ongoing fix. # The constraint can be removed once a release (pip-tools > 7.5.1) is available with support for pip 25.3 diff --git a/requirements/constraints.txt b/requirements/constraints.txt index 96ee4cbcbf80..c96838f93777 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -61,7 +61,7 @@ numpy<2.0.0 # Date: 2023-09-18 # pinning this version to avoid updates while the library is being developed # Issue for unpinning: https://github.com/openedx/edx-platform/issues/35269 -openedx-learning==0.29.1 +openedx-learning==0.30.0 # Date: 2023-11-29 # Open AI version 1.0.0 dropped support for openai.ChatCompletion which is currently in use in enterprise. diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 1f6eb49a2ad3..f4fa71e700fb 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -854,7 +854,7 @@ openedx-filters==2.1.0 # ora2 openedx-forum==0.3.8 # via -r requirements/edx/kernel.in -openedx-learning==0.29.1 +openedx-learning==0.30.0 # via # -c requirements/constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index b3fc16072930..3afded2e283b 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1418,7 +1418,7 @@ openedx-forum==0.3.8 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt -openedx-learning==0.29.1 +openedx-learning==0.30.0 # via # -c requirements/constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index ac81bd89e8bd..e25735f3961c 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -1032,7 +1032,7 @@ openedx-filters==2.1.0 # ora2 openedx-forum==0.3.8 # via -r requirements/edx/base.txt -openedx-learning==0.29.1 +openedx-learning==0.30.0 # via # -c requirements/constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 5cfe412a7fbd..d0faa2471265 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1078,7 +1078,7 @@ openedx-filters==2.1.0 # ora2 openedx-forum==0.3.8 # via -r requirements/edx/base.txt -openedx-learning==0.29.1 +openedx-learning==0.30.0 # via # -c requirements/constraints.txt # -r requirements/edx/base.txt From 711ae031dd124894c93950799c4b1e68b9fa540e Mon Sep 17 00:00:00 2001 From: Farhaan Bukhsh Date: Sat, 8 Nov 2025 02:44:52 +0530 Subject: [PATCH 05/21] chore: Adds sandbox requirements to ulmo (#37584) Signed-off-by: Farhaan Bukhsh --- requirements/edx-sandbox/README.rst | 18 +++++ requirements/edx-sandbox/releases/ulmo.txt | 90 ++++++++++++++++++++++ 2 files changed, 108 insertions(+) create mode 100644 requirements/edx-sandbox/releases/ulmo.txt diff --git a/requirements/edx-sandbox/README.rst b/requirements/edx-sandbox/README.rst index 4d628f3e2add..d4b1ab8199a1 100644 --- a/requirements/edx-sandbox/README.rst +++ b/requirements/edx-sandbox/README.rst @@ -74,3 +74,21 @@ releases/sumac.txt .. _Python changelog: https://docs.python.org/3.11/whatsnew/changelog.html .. _SciPy changelog: https://docs.scipy.org/doc/scipy/release.html .. _NumPy changelog: https://numpy.org/doc/stable/release.html + +releases/teak.txt +------------------ + +* Frozen at the time of the Teak release +* Supports Python 3.11 and Python 3.12 +* SciPy is upgraded from 1.14.1 to 1.15.2 + +.. _SciPy changelog: https://docs.scipy.org/doc/scipy/release.html + +releases/ulmo.txt +------------------ + +* Frozen at the time of the Ulmo release +* Supports Python 3.11 and Python 3.12 +* SciPy is upgraded from 1.15.2 to 1.16.3 + +.. _SciPy changelog: https://docs.scipy.org/doc/scipy/release.html diff --git a/requirements/edx-sandbox/releases/ulmo.txt b/requirements/edx-sandbox/releases/ulmo.txt new file mode 100644 index 000000000000..887f5cc1beaf --- /dev/null +++ b/requirements/edx-sandbox/releases/ulmo.txt @@ -0,0 +1,90 @@ +# +# This file is autogenerated by pip-compile with Python 3.11 +# by the following command: +# +# make upgrade +# +cffi==2.0.0 + # via cryptography +chem==2.0.0 + # via -r requirements/edx-sandbox/base.in +click==8.3.0 + # via nltk +codejail-includes==2.0.0 + # via -r requirements/edx-sandbox/base.in +contourpy==1.3.3 + # via matplotlib +cryptography==45.0.7 + # via + # -c requirements/constraints.txt + # -r requirements/edx-sandbox/base.in +cycler==0.12.1 + # via matplotlib +fonttools==4.60.1 + # via matplotlib +joblib==1.5.2 + # via nltk +kiwisolver==1.4.9 + # via matplotlib +lxml[html-clean]==5.3.2 + # via + # -c requirements/constraints.txt + # -r requirements/edx-sandbox/base.in + # lxml-html-clean + # openedx-calc +lxml-html-clean==0.4.3 + # via lxml +markupsafe==3.0.3 + # via + # chem + # openedx-calc +matplotlib==3.10.7 + # via -r requirements/edx-sandbox/base.in +mpmath==1.3.0 + # via sympy +networkx==3.5 + # via -r requirements/edx-sandbox/base.in +nltk==3.9.2 + # via + # -r requirements/edx-sandbox/base.in + # chem +numpy==1.26.4 + # via + # -c requirements/constraints.txt + # chem + # contourpy + # matplotlib + # openedx-calc + # scipy +openedx-calc==4.0.2 + # via -r requirements/edx-sandbox/base.in +packaging==25.0 + # via matplotlib +pillow==12.0.0 + # via matplotlib +pycparser==2.23 + # via cffi +pyparsing==3.2.5 + # via + # -r requirements/edx-sandbox/base.in + # chem + # matplotlib + # openedx-calc +python-dateutil==2.9.0.post0 + # via matplotlib +random2==1.0.2 + # via -r requirements/edx-sandbox/base.in +regex==2025.10.23 + # via nltk +scipy==1.16.3 + # via + # -r requirements/edx-sandbox/base.in + # chem +six==1.17.0 + # via python-dateutil +sympy==1.14.0 + # via + # -r requirements/edx-sandbox/base.in + # openedx-calc +tqdm==4.67.1 + # via nltk From 3cf5e34d94297f9652963945c7852de24efe2541 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Chris=20Ch=C3=A1vez?= Date: Thu, 13 Nov 2025 11:05:56 -0500 Subject: [PATCH 06/21] fix: Call `LIBRARY_CONTAINER_PUBLISHED` for parent of containers (#37622) (#37629) Calls `LIBRARY_CONTAINER_PUBLISHED` when publishing a container that is child of another container. --- .../djangoapps/content_libraries/tasks.py | 9 ++++ .../content_libraries/tests/test_events.py | 47 +++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index 93d9fef725ec..d4e6196e03a2 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -127,6 +127,15 @@ def send_events_after_publish(publish_log_pk: int, library_key_str: str) -> None elif hasattr(record.entity, "container"): container_key = api.library_container_locator(library_key, record.entity.container) affected_containers.add(container_key) + + try: + # We do need to notify listeners that the parent container(s) have changed, + # e.g. so the search index can update the "has_unpublished_changes" + for parent_container in api.get_containers_contains_item(container_key): + affected_containers.add(parent_container.container_key) + except api.ContentLibraryContainerNotFound: + # The deleted children remains in the entity, so, in this case, the container may not be found. + pass else: log.warning( f"PublishableEntity {record.entity.pk} / {record.entity.key} was modified during publish operation " diff --git a/openedx/core/djangoapps/content_libraries/tests/test_events.py b/openedx/core/djangoapps/content_libraries/tests/test_events.py index 88d426d3ef06..975cfbafb4d9 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_events.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_events.py @@ -449,6 +449,53 @@ def test_publish_container(self) -> None: c2_after = self._get_container(container2["id"]) assert c2_after["has_unpublished_changes"] + def test_publish_child_container(self): + """ + Test the events that get emitted when we publish the changes to a container that is child of another container + """ + # Create some containers + unit = self._create_container(self.lib1_key, "unit", display_name="Alpha Unit", slug=None) + subsection = self._create_container(self.lib1_key, "subsection", display_name="Bravo Subsection", slug=None) + + # Add one container as child + self._add_container_children(subsection["id"], children_ids=[unit["id"]]) + + # At first everything is unpublished: + c1_before = self._get_container(unit["id"]) + assert c1_before["has_unpublished_changes"] + c2_before = self._get_container(subsection["id"]) + assert c2_before["has_unpublished_changes"] + + # clear event log after the initial mock data setup is complete: + self.clear_events() + + # Now publish only the unit + self._publish_container(unit["id"]) + + # Now it is published: + c1_after = self._get_container(unit["id"]) + assert c1_after["has_unpublished_changes"] is False + + # And publish events were emitted: + self.expect_new_events( + { # An event for the unit being published: + "signal": LIBRARY_CONTAINER_PUBLISHED, + "library_container": LibraryContainerData( + container_key=LibraryContainerLocator.from_string(unit["id"]), + ), + }, + { # An event for parent (subsection): + "signal": LIBRARY_CONTAINER_PUBLISHED, + "library_container": LibraryContainerData( + container_key=LibraryContainerLocator.from_string(subsection["id"]), + ), + }, + ) + + # note that subsection is still unpublished + c2_after = self._get_container(subsection["id"]) + assert c2_after["has_unpublished_changes"] + def test_restore_unit(self) -> None: """ Test restoring a deleted unit via the "restore" API. From d9ec5be4a7195210ca34f608af6bcf3acbc9c3d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mar=C3=ADa=20Fernanda=20Magallanes?= <35668326+MaferMazu@users.noreply.github.com> Date: Thu, 20 Nov 2025 14:29:09 -0500 Subject: [PATCH 07/21] [Backport FC-0099] feat: add openedx-authz and update libraries enforcement points (#37633) * feat: filter libraries based on user-role scopes (#37564) (cherry picked from commit 6c6fc5d55143308f240457ed230a18605ff53ae9) * feat: add openedx-authz to user_can_create_library and require_permission_for_library_key (#37501) * feat: add the authz check to the library api function feat: add the authz publish check in rest_api blocks and containers feat: add the authz checks in libraries and refactor feat: add collections checks feat: update enforcement in serializer file refactor: refactor the permission check functions fix: fix value error fix: calling the queries twice * test: add structure for test and apply feedback refactor: refactor the tests and apply feedback fix: apply feedback Revert "refactor: refactor the tests and apply feedback" This reverts commit aa0bd527dd7bc7dec4a7ad7adb41a3c932f4a587. refactor: use constants and avoid mapping test: fix the test to have them in order docs: about we rely on bridgekeeper and the old check for two cases docs: update openedx/core/djangoapps/content_libraries/api/libraries.py Co-authored-by: Maria Grimaldi (Majo) refactor: use global scope wildcard instead of * refactor: allow receiving PermissionData objects refactor: do not inherit from BaseRolesTestCase to favor CL setup methods If both BaseRolesTestCase and ContentLibrariesRestApiTest define a method with the same name (e.g., setUp()), Python will use the one found first in the MRO, which is the one in BaseRolesTestCase because it is listed first in the class definition leading to unexpected behavior. refactor: remove unnecessary imports and indent * chore: bump openedx-authz version (cherry picked from commit f4f14a69874d5804ef33778486f7d6cab400e206) * feat: Upgrade Python dependency openedx-authz (#37652) * feat: Upgrade Python dependency openedx-authz handle cache invalidation Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master` * fix: update the num of queries in tests --------- Co-authored-by: MaferMazu <35668326+MaferMazu@users.noreply.github.com> Co-authored-by: Maria Fernanda Magallanes Zubillaga (cherry picked from commit 122b4e072d6199b51533c29b4fb8e29546d1dc5d) * chore: update requirements to fix the inconsistency --------- Co-authored-by: Maria Grimaldi (Majo) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../content_libraries/api/libraries.py | 123 ++- .../content_libraries/api/permissions.py | 10 + .../content_libraries/permissions.py | 158 +++- .../content_libraries/rest_api/blocks.py | 3 +- .../content_libraries/rest_api/collections.py | 28 +- .../content_libraries/rest_api/containers.py | 3 +- .../content_libraries/rest_api/libraries.py | 11 +- .../content_libraries/rest_api/serializers.py | 4 +- .../tests/test_content_libraries.py | 747 +++++++++++++++++- .../rest_api/v1/tests/test_views.py | 32 +- requirements/common_constraints.txt | 2 +- requirements/edx/base.txt | 3 +- requirements/edx/development.txt | 3 +- requirements/edx/doc.txt | 3 +- requirements/edx/testing.txt | 3 +- scripts/user_retirement/requirements/base.txt | 1 + 16 files changed, 1091 insertions(+), 43 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index 8d32e4dbc015..66281addb643 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -53,13 +53,11 @@ from django.db import IntegrityError, transaction from django.db.models import Q, QuerySet from django.utils.translation import gettext as _ -from opaque_keys.edx.locator import ( - LibraryLocatorV2, - LibraryUsageLocatorV2, -) -from openedx_events.content_authoring.data import ( - ContentLibraryData, -) +from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 +from openedx_authz import api as authz_api +from openedx_authz.api import assign_role_to_user_in_scope +from openedx_authz.constants import permissions as authz_permissions +from openedx_events.content_authoring.data import ContentLibraryData from openedx_events.content_authoring.signals import ( CONTENT_LIBRARY_CREATED, CONTENT_LIBRARY_DELETED, @@ -70,7 +68,6 @@ from organizations.models import Organization from user_tasks.models import UserTaskArtifact, UserTaskStatus from xblock.core import XBlock -from openedx_authz.api import assign_role_to_user_in_scope from openedx.core.types import User as UserType @@ -78,6 +75,7 @@ from ..constants import ALL_RIGHTS_RESERVED from ..models import ContentLibrary, ContentLibraryPermission from .exceptions import LibraryAlreadyExists, LibraryPermissionIntegrityError +from .permissions import LEGACY_LIB_PERMISSIONS log = logging.getLogger(__name__) @@ -109,6 +107,7 @@ "revert_changes", "get_backup_task_status", "assign_library_role_to_user", + "user_has_permission_across_lib_authz_systems", ] @@ -245,7 +244,18 @@ def user_can_create_library(user: AbstractUser) -> bool: """ Check if the user has permission to create a content library. """ - return user.has_perm(permissions.CAN_CREATE_CONTENT_LIBRARY) + library_permission = permissions.CAN_CREATE_CONTENT_LIBRARY + lib_permission_in_authz = _transform_legacy_lib_permission_to_authz_permission(library_permission) + # The authz_api.is_user_allowed check only validates permissions within a specific library context. Since + # creating a library is not tied to an existing one, we use user.has_perm (via Bridgekeeper) to check if the user + # can create libraries, meaning they have the course creator role. In the future, this should rely on a global (*) + # role defined in the Authorization Framework for instance-level resource creation. + has_perms = user.has_perm(library_permission) or authz_api.is_user_allowed( + user, + lib_permission_in_authz, + authz_api.data.GLOBAL_SCOPE_WILDCARD, + ) + return has_perms def get_libraries_for_user(user, org=None, text_search=None, order=None) -> QuerySet[ContentLibrary]: @@ -267,7 +277,11 @@ def get_libraries_for_user(user, org=None, text_search=None, order=None) -> Quer Q(learning_package__description__icontains=text_search) ) - filtered = permissions.perms[permissions.CAN_VIEW_THIS_CONTENT_LIBRARY].filter(user, qs) + # Using distinct() temporarily to avoid duplicate results caused by overlapping permission checks + # between Bridgekeeper and the new authorization framework. This ensures correct results for now, + # but it should be removed once Bridgekeeper support is fully dropped and all permission logic + # is handled through openedx-authz. + filtered = permissions.perms[permissions.CAN_VIEW_THIS_CONTENT_LIBRARY].filter(user, qs).distinct() if order: order_query = 'learning_package__' @@ -332,7 +346,7 @@ def require_permission_for_library_key(library_key: LibraryLocatorV2, user: User library_obj = ContentLibrary.objects.get_by_key(library_key) # obj should be able to read any valid model object but mypy thinks it can only be # "User | AnonymousUser | None" - if not user.has_perm(permission, obj=library_obj): # type:ignore[arg-type] + if not user_has_permission_across_lib_authz_systems(user, permission, library_obj): raise PermissionDenied return library_obj @@ -750,3 +764,90 @@ def get_backup_task_status( result['file'] = artifact.file return result + + +def _transform_legacy_lib_permission_to_authz_permission(permission: str) -> str: + """ + Transform a legacy content library permission to an openedx-authz permission. + """ + # There is no dedicated permission or role for can_create_content_library in openedx-authz yet, + # so we reuse the same permission to rely on user.has_perm via Bridgekeeper. + return { + permissions.CAN_CREATE_CONTENT_LIBRARY: permissions.CAN_CREATE_CONTENT_LIBRARY, + permissions.CAN_DELETE_THIS_CONTENT_LIBRARY: authz_permissions.DELETE_LIBRARY.identifier, + permissions.CAN_EDIT_THIS_CONTENT_LIBRARY: authz_permissions.EDIT_LIBRARY_CONTENT.identifier, + permissions.CAN_EDIT_THIS_CONTENT_LIBRARY_TEAM: authz_permissions.MANAGE_LIBRARY_TEAM.identifier, + permissions.CAN_VIEW_THIS_CONTENT_LIBRARY: authz_permissions.VIEW_LIBRARY.identifier, + permissions.CAN_VIEW_THIS_CONTENT_LIBRARY_TEAM: authz_permissions.VIEW_LIBRARY_TEAM.identifier, + }.get(permission, permission) + + +def _transform_authz_permission_to_legacy_lib_permission(permission: str) -> str: + """ + Transform an openedx-authz permission to a legacy content library permission. + """ + return { + authz_permissions.PUBLISH_LIBRARY_CONTENT.identifier: permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, + authz_permissions.CREATE_LIBRARY_COLLECTION.identifier: permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, + authz_permissions.EDIT_LIBRARY_COLLECTION.identifier: permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, + authz_permissions.DELETE_LIBRARY_COLLECTION.identifier: permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, + }.get(permission, permission) + + +def user_has_permission_across_lib_authz_systems( + user: UserType, + permission: str | authz_api.data.PermissionData, + library_obj: ContentLibrary, +) -> bool: + """ + Check whether a user has a given permission on a content library across both the + legacy edx-platform permission system and the newer openedx-authz system. + + The provided permission name is normalized to both systems (legacy and authz), and + authorization is granted if either: + - the user holds the legacy object-level permission on the ContentLibrary instance, or + - the openedx-authz API allows the user for the corresponding permission on the library. + + **Note:** + Temporary: this function uses Bridgekeeper-based logic for cases not yet modeled in openedx-authz. + + Current gaps covered here: + - CAN_CREATE_CONTENT_LIBRARY: we call user.has_perm via Bridgekeeper to verify the user is a course creator. + - CAN_VIEW_THIS_CONTENT_LIBRARY: we respect the allow_public_read flag via Bridgekeeper. + + Replace these with authz_api.is_user_allowed once openedx-authz supports + these conditions natively (including global (*) roles). + + Args: + user: The Django user (or user-like object) to check. + permission: The permission identifier (either a legacy codename or an openedx-authz name). + library_obj: The ContentLibrary instance to check against. + + Returns: + bool: True if the user is authorized by either system; otherwise False. + """ + if isinstance(permission, authz_api.data.PermissionData): + permission = permission.identifier + if _is_legacy_permission(permission): + legacy_permission = permission + authz_permission = _transform_legacy_lib_permission_to_authz_permission(permission) + else: + authz_permission = permission + legacy_permission = _transform_authz_permission_to_legacy_lib_permission(permission) + return ( + # Check both the legacy and the new openedx-authz permissions + user.has_perm(perm=legacy_permission, obj=library_obj) + or authz_api.is_user_allowed( + user, + authz_permission, + str(library_obj.library_key), + ) + ) + + +def _is_legacy_permission(permission: str) -> bool: + """ + Determine if the specified library permission is part of the legacy + or the new openedx-authz system. + """ + return permission in LEGACY_LIB_PERMISSIONS diff --git a/openedx/core/djangoapps/content_libraries/api/permissions.py b/openedx/core/djangoapps/content_libraries/api/permissions.py index 6064b80d6f9e..5b8bd4ba7e1a 100644 --- a/openedx/core/djangoapps/content_libraries/api/permissions.py +++ b/openedx/core/djangoapps/content_libraries/api/permissions.py @@ -12,3 +12,13 @@ CAN_VIEW_THIS_CONTENT_LIBRARY, CAN_VIEW_THIS_CONTENT_LIBRARY_TEAM ) + +LEGACY_LIB_PERMISSIONS = frozenset({ + CAN_CREATE_CONTENT_LIBRARY, + CAN_DELETE_THIS_CONTENT_LIBRARY, + CAN_EDIT_THIS_CONTENT_LIBRARY, + CAN_EDIT_THIS_CONTENT_LIBRARY_TEAM, + CAN_LEARN_FROM_THIS_CONTENT_LIBRARY, + CAN_VIEW_THIS_CONTENT_LIBRARY, + CAN_VIEW_THIS_CONTENT_LIBRARY_TEAM, +}) diff --git a/openedx/core/djangoapps/content_libraries/permissions.py b/openedx/core/djangoapps/content_libraries/permissions.py index 4e72381986ed..c3a8b68c947c 100644 --- a/openedx/core/djangoapps/content_libraries/permissions.py +++ b/openedx/core/djangoapps/content_libraries/permissions.py @@ -2,8 +2,12 @@ Permissions for Content Libraries (v2, Learning-Core-based) """ from bridgekeeper import perms, rules -from bridgekeeper.rules import Attribute, ManyRelation, Relation, blanket_rule, in_current_groups +from bridgekeeper.rules import Attribute, ManyRelation, Relation, blanket_rule, in_current_groups, Rule from django.conf import settings +from django.db.models import Q + +from openedx_authz import api as authz_api +from openedx_authz.constants.permissions import VIEW_LIBRARY from openedx.core.djangoapps.content_libraries.models import ContentLibraryPermission @@ -54,6 +58,154 @@ def is_course_creator(user): return get_course_creator_status(user) == 'granted' + +class HasPermissionInContentLibraryScope(Rule): + """Bridgekeeper rule that checks content library permissions via the openedx-authz system. + + This rule integrates the openedx-authz authorization system (backed by Casbin) with + Bridgekeeper's declarative permission system. It checks if a user has been granted a + specific permission (action) through their role assignments in the authorization system. + + The rule works by: + 1. Querying the authorization system to find library scopes where the user has this permission + 2. Parsing the library keys (org/slug) from the scopes + 3. Building database filters to match ContentLibrary models with those org/slug combinations + + Attributes: + permission (PermissionData): The permission object representing the action to check + (e.g., 'view', 'edit'). This is used to look up scopes in the authorization system. + + filter_keys (list[str]): The Django model fields to use when building QuerySet filters. + Defaults to ['org', 'slug'] for ContentLibrary models. + + These fields are used to construct the Q object filters that match libraries + based on the parsed components from library keys in authorization scopes. + + For ContentLibrary, library keys have the format 'lib:ORG:SLUG', which maps to: + - 'org' -> filters on org__short_name (related Organization model) + - 'slug' -> filters on slug field + + If filtering by different fields is needed, pass a custom list. For example: + - ['org', 'slug'] - default for ContentLibrary (filters by org and slug) + - ['id'] - filter by primary key (for other models) + + Examples: + Basic usage with default filter_keys: + >>> from bridgekeeper import perms + >>> from openedx.core.djangoapps.content_libraries.permissions import HasPermissionInContentLibraryScope + >>> + >>> # Uses default filter_keys=['org', 'slug'] for ContentLibrary + >>> can_view = HasPermissionInContentLibraryScope('view_library') + >>> perms['libraries.view_library'] = can_view + + Compound permissions with boolean operators: + >>> from bridgekeeper.rules import Attribute + >>> + >>> is_active = Attribute('is_active', True) + >>> is_staff = Attribute('is_staff', True) + >>> can_view = HasPermissionInContentLibraryScope('view_library') + >>> + >>> # User must be active AND (staff OR have explicit permission) + >>> perms['libraries.view_library'] = is_active & (is_staff | can_view) + + QuerySet filtering (efficient, database-level): + >>> from openedx.core.djangoapps.content_libraries.models import ContentLibrary + >>> + >>> # Gets all libraries user can view in a single SQL query + >>> visible_libraries = perms['libraries.view_library'].filter( + ... request.user, + ... ContentLibrary.objects.all() + ... ) + + Individual object checks: + >>> library = ContentLibrary.objects.get(org__short_name='DemoX', slug='CSPROB') + >>> if perms['libraries.view_library'].check(request.user, library): + ... # User can view this specific library + + Note: + The library keys in authorization scopes must have the format 'lib:ORG:SLUG' + to match the ContentLibrary model's org.short_name and slug fields. + For example, scope 'lib:DemoX:CSPROB' matches a library with + org.short_name='DemoX' and slug='CSPROB'. + """ + + def __init__(self, permission: authz_api.PermissionData, filter_keys: list[str] | None = None): + """Initialize the rule with the action and filter keys to filter on. + + Args: + permission (PermissionData): The permission to check (e.g., 'view', 'edit'). + filter_keys (list[str]): The model fields to filter on when building QuerySet filters. + Defaults to ['org', 'slug'] for ContentLibrary. + """ + self.permission = permission + self.filter_keys = filter_keys if filter_keys is not None else ["org", "slug"] + + def query(self, user): + """Convert this rule to a Django Q object for QuerySet filtering. + + Args: + user: The Django user object (must have a 'username' attribute). + + Returns: + Q: A Django Q object that can be used to filter a QuerySet. + The Q object combines multiple conditions using OR (|) operators, + where each condition matches a library's org and slug fields: + Q(org__short_name='OrgA' & slug='lib-a') | Q(org__short_name='OrgB' & slug='lib-b') + + Example: + >>> # User has 'view' permission in scopes: ['lib:OrgA:lib-a', 'lib:OrgB:lib-b'] + >>> rule = HasPermissionInContentLibraryScope('view', filter_keys=['org', 'slug']) + >>> q = rule.query(user) + >>> # Results in: Q(org__short_name='OrgA', slug='lib-a') | Q(org__short_name='OrgB', slug='lib-b') + >>> + >>> # Apply to queryset + >>> libraries = ContentLibrary.objects.filter(q) + >>> # SQL: SELECT * FROM content_library + >>> # WHERE (org.short_name='OrgA' AND slug='lib-a') + >>> # OR (org.short_name='OrgB' AND slug='lib-b') + """ + scopes = authz_api.get_scopes_for_user_and_permission( + user.username, + self.permission.identifier + ) + + library_keys = [scope.library_key for scope in scopes] + + if not library_keys: + return Q(pk__in=[]) # No access, return Q that matches nothing + + # Build Q object: OR together (org AND slug) conditions for each library + query = Q() + for library_key in library_keys: + query |= Q(org__short_name=library_key.org, slug=library_key.slug) + + return query + + def check(self, user, instance, *args, **kwargs): # pylint: disable=arguments-differ + """Check if user has permission for a specific object instance. + + This method is used for checking permission on individual objects rather + than filtering a QuerySet. It extracts the scope from the object and + checks if the user has the required permission in that scope via Casbin. + + Args: + user: The Django user object (must have a 'username' attribute). + instance: The Django model instance to check permission for. + *args: Additional positional arguments (for compatibility with parent signature). + **kwargs: Additional keyword arguments (for compatibility with parent signature). + + Returns: + bool: True if the user has the permission in the object's scope, + False otherwise. + + Example: + >>> rule = HasPermissionInContentLibraryScope('view') + >>> can_view = rule.check(user, library) + >>> # Checks if user has 'view' permission in scope 'lib:DemoX:CSPROB' + """ + return authz_api.is_user_allowed(user.username, self.permission.identifier, str(instance.library_key)) + + ########################### Permissions ########################### # Is the user allowed to view XBlocks from the specified content library @@ -87,7 +239,9 @@ def is_course_creator(user): is_global_staff | # Libraries with "public read" permissions can be accessed only by course creators (Attribute('allow_public_read', True) & is_course_creator) | - # Otherwise the user must be part of the library's team + # Users can access libraries within their authorized scope (via Casbin/role-based permissions) + HasPermissionInContentLibraryScope(VIEW_LIBRARY) | + # Fallback to: the user must be part of the library's team (legacy permission system) has_explicit_read_permission_for_library ) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py index b93b48e5ad86..7aa15f6e7834 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py @@ -9,6 +9,7 @@ from django.utils.decorators import method_decorator from drf_yasg.utils import swagger_auto_schema from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 +from openedx_authz.constants import permissions as authz_permissions from openedx_learning.api import authoring as authoring_api from rest_framework import status from rest_framework.exceptions import NotFound, ValidationError @@ -238,7 +239,7 @@ def post(self, request, usage_key_str): api.require_permission_for_library_key( key.lib_key, request.user, - permissions.CAN_EDIT_THIS_CONTENT_LIBRARY + authz_permissions.PUBLISH_LIBRARY_CONTENT ) api.publish_component_changes(key, request.user) return Response({}) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/collections.py b/openedx/core/djangoapps/content_libraries/rest_api/collections.py index d893d766d80f..f4d579aa04a2 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/collections.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/collections.py @@ -13,6 +13,7 @@ from rest_framework.status import HTTP_204_NO_CONTENT from opaque_keys.edx.locator import LibraryLocatorV2 +from openedx_authz.constants import permissions as authz_permissions from openedx_learning.api import authoring as authoring_api from openedx_learning.api.authoring_models import Collection @@ -56,7 +57,6 @@ def get_content_library(self) -> ContentLibrary: if self.request.method in ['OPTIONS', 'GET'] else permissions.CAN_EDIT_THIS_CONTENT_LIBRARY ) - self._content_library = api.require_permission_for_library_key( library_key, self.request.user, @@ -110,6 +110,11 @@ def create(self, request: RestRequest, *args, **kwargs) -> Response: Create a Collection that belongs to a Content Library """ content_library = self.get_content_library() + api.require_permission_for_library_key( + content_library.library_key, + request.user, + authz_permissions.CREATE_LIBRARY_COLLECTION + ) create_serializer = ContentLibraryCollectionUpdateSerializer(data=request.data) create_serializer.is_valid(raise_exception=True) @@ -144,6 +149,11 @@ def partial_update(self, request: RestRequest, *args, **kwargs) -> Response: Update a Collection that belongs to a Content Library """ content_library = self.get_content_library() + api.require_permission_for_library_key( + content_library.library_key, + request.user, + authz_permissions.EDIT_LIBRARY_COLLECTION + ) collection_key = kwargs["key"] update_serializer = ContentLibraryCollectionUpdateSerializer( @@ -165,6 +175,12 @@ def destroy(self, request: RestRequest, *args, **kwargs) -> Response: """ Soft-deletes a Collection that belongs to a Content Library """ + content_library = self.get_content_library() + api.require_permission_for_library_key( + content_library.library_key, + request.user, + authz_permissions.DELETE_LIBRARY_COLLECTION + ) collection = super().get_object() assert collection.learning_package_id authoring_api.delete_collection( @@ -181,6 +197,11 @@ def restore(self, request: RestRequest, *args, **kwargs) -> Response: Restores a soft-deleted Collection that belongs to a Content Library """ content_library = self.get_content_library() + api.require_permission_for_library_key( + content_library.library_key, + request.user, + authz_permissions.EDIT_LIBRARY_COLLECTION + ) assert content_library.learning_package_id collection_key = kwargs["key"] authoring_api.restore_collection( @@ -198,6 +219,11 @@ def update_items(self, request: RestRequest, *args, **kwargs) -> Response: Collection and items must all be part of the given library/learning package. """ content_library = self.get_content_library() + api.require_permission_for_library_key( + content_library.library_key, + request.user, + authz_permissions.EDIT_LIBRARY_COLLECTION + ) collection_key = kwargs["key"] serializer = ContentLibraryItemKeysSerializer(data=request.data) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/containers.py b/openedx/core/djangoapps/content_libraries/rest_api/containers.py index 67070a0a82f9..c60c40b9802d 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/containers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/containers.py @@ -12,6 +12,7 @@ from drf_yasg import openapi from opaque_keys.edx.locator import LibraryLocatorV2, LibraryContainerLocator +from openedx_authz.constants import permissions as authz_permissions from openedx_learning.api import authoring as authoring_api from rest_framework.generics import GenericAPIView from rest_framework.response import Response @@ -379,7 +380,7 @@ def post(self, request: RestRequest, container_key: LibraryContainerLocator) -> api.require_permission_for_library_key( container_key.lib_key, request.user, - permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, + authz_permissions.PUBLISH_LIBRARY_CONTENT ) api.publish_container_changes(container_key, request.user.id) # If we need to in the future, we could return a list of all the child containers/components that were diff --git a/openedx/core/djangoapps/content_libraries/rest_api/libraries.py b/openedx/core/djangoapps/content_libraries/rest_api/libraries.py index 9f6cca19947a..2d50fa6c8644 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/libraries.py @@ -82,6 +82,7 @@ from user_tasks.models import UserTaskStatus from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 +from openedx_authz.constants import permissions as authz_permissions from organizations.api import ensure_organization from organizations.exceptions import InvalidOrganizationException from organizations.models import Organization @@ -219,7 +220,7 @@ def post(self, request): """ Create a new content library. """ - if not request.user.has_perm(permissions.CAN_CREATE_CONTENT_LIBRARY): + if not api.user_can_create_library(request.user): raise PermissionDenied serializer = ContentLibraryMetadataSerializer(data=request.data) serializer.is_valid(raise_exception=True) @@ -479,7 +480,11 @@ def post(self, request, lib_key_str): descendants. """ key = LibraryLocatorV2.from_string(lib_key_str) - api.require_permission_for_library_key(key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) + api.require_permission_for_library_key( + key, + request.user, + authz_permissions.PUBLISH_LIBRARY_CONTENT + ) api.publish_changes(key, request.user.id) return Response({}) @@ -838,7 +843,7 @@ def post(self, request): """ Restore a library from a backup file. """ - if not request.user.has_perm(permissions.CAN_CREATE_CONTENT_LIBRARY): + if not api.user_can_create_library(request.user): raise PermissionDenied serializer = LibraryRestoreFileSerializer(data=request.data) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py index a1e24c6a64a4..c0bf07d087fc 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/serializers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/serializers.py @@ -14,6 +14,7 @@ from user_tasks.models import UserTaskStatus from openedx.core.djangoapps.content_libraries.tasks import LibraryRestoreTask +from openedx.core.djangoapps.content_libraries import api from openedx.core.djangoapps.content_libraries.api.containers import ContainerType from openedx.core.djangoapps.content_libraries.constants import ALL_RIGHTS_RESERVED, LICENSE_OPTIONS from openedx.core.djangoapps.content_libraries.models import ( @@ -75,7 +76,8 @@ def get_can_edit_library(self, obj): return False library_obj = ContentLibrary.objects.get_by_key(obj.key) - return user.has_perm(permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, obj=library_obj) + return api.user_has_permission_across_lib_authz_systems( + user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, library_obj) class ContentLibraryUpdateSerializer(serializers.Serializer): diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py index c4f61f47e254..91a9c29a3754 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -12,14 +12,17 @@ import ddt import tomlkit +from bridgekeeper import perms from django.core.files.uploadedfile import SimpleUploadedFile from django.contrib.auth.models import Group +from django.db.models import Q from django.test import override_settings from django.test.client import Client from freezegun import freeze_time -from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 +from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2, LibraryCollectionLocator from organizations.models import Organization from rest_framework.test import APITestCase +from rest_framework import status from openedx_learning.api.authoring_models import LearningPackage from user_tasks.models import UserTaskStatus, UserTaskArtifact @@ -33,10 +36,15 @@ URL_BLOCK_XBLOCK_HANDLER, ContentLibrariesRestApiTest, ) +from openedx_authz import api as authz_api +from openedx_authz.constants import roles +from openedx_authz.engine.enforcer import AuthzEnforcer from openedx.core.djangoapps.xblock import api as xblock_api from openedx.core.djangolib.testing.utils import skip_unless_cms +from openedx_authz.constants.permissions import VIEW_LIBRARY -from ..models import ContentLibrary +from ..models import ContentLibrary, ContentLibraryPermission +from ..permissions import CAN_VIEW_THIS_CONTENT_LIBRARY, HasPermissionInContentLibraryScope @skip_unless_cms @@ -1217,6 +1225,462 @@ def test_uncaught_error_creates_error_log(self): self.assertEqual(task_data, expected) +@skip_unless_cms +class ContentLibrariesAuthZTestCase(ContentLibrariesRestApiTest): + """ + Tests for Content Libraries AuthZ integration via openedx-authz. + + These tests verify the HasPermissionInContentLibraryScope Bridgekeeper rule + integrates correctly with the openedx-authz authorization system (Casbin). + See: https://github.com/openedx/openedx-authz/ + + IMPORTANT: These tests explicitly remove legacy ContentLibraryPermission grants + to ensure ONLY the AuthZ system is being tested, not the legacy fallback. + """ + + def setUp(self): + super().setUp() + # The parent class provides self.user (a staff user) and self.organization + # Set up admin_user as an alias to self.user for test readability + self.admin_user = self.user + # Set up org_short_name for convenience + self.org_short_name = self.organization.short_name + + def test_authz_scope_filters_by_authorized_libraries(self): + """ + Test that HasPermissionInContentLibraryScope rule filters libraries + based on authorized org/slug combinations. + + Given: + - 3 libraries: lib1 (org1), lib2 (org2), lib3 (org1) + - User authorized for lib1 and lib2 only via AuthZ (NO legacy permissions) + + Expected: + - Filter returns exactly 2 libraries (lib1 and lib2) + - lib3 is excluded (same org as lib1, but different slug) + - Correct org/slug combinations are matched + """ + user = UserFactory.create(username="scope_user", is_staff=False) + + Organization.objects.get_or_create(short_name="org1", defaults={"name": "Org 1"}) + Organization.objects.get_or_create(short_name="org2", defaults={"name": "Org 2"}) + + with self.as_user(self.admin_user): + lib1 = self._create_library(slug="lib1", org="org1", title="Library 1") + lib2 = self._create_library(slug="lib2", org="org2", title="Library 2") + self._create_library(slug="lib3", org="org1", title="Library 3") + + # CRITICAL: Ensure user has NO legacy permissions (test ONLY AuthZ filtering) + ContentLibraryPermission.objects.filter(user=user).delete() + + with patch( + 'openedx_authz.api.get_scopes_for_user_and_permission' + ) as mock_get_scopes: + # Mock: User authorized for lib1 (org1:lib1) and lib2 (org2:lib2) only, NOT lib3 + mock_scope1 = type('Scope', (), {'library_key': LibraryLocatorV2.from_string(lib1['id'])})() + mock_scope2 = type('Scope', (), {'library_key': LibraryLocatorV2.from_string(lib2['id'])})() + mock_get_scopes.return_value = [mock_scope1, mock_scope2] + + all_libs = ContentLibrary.objects.filter(slug__in=['lib1', 'lib2', 'lib3']) + filtered = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].filter(user, all_libs).distinct() + + # TEST: Verify exactly 2 libraries returned (lib1 and lib2, not lib3) + self.assertEqual(filtered.count(), 2, "Should return exactly 2 authorized libraries") + + # TEST: Verify correct libraries are included/excluded + slugs = set(filtered.values_list('slug', flat=True)) + self.assertIn('lib1', slugs, "lib1 (org1:lib1) should be included") + self.assertIn('lib2', slugs, "lib2 (org2:lib2) should be included") + self.assertNotIn('lib3', slugs, "lib3 (org1:lib3) should be excluded") + + # TEST: Verify the org/slug combinations match + lib1_result = filtered.get(slug='lib1') + lib2_result = filtered.get(slug='lib2') + self.assertEqual(lib1_result.org.short_name, 'org1') + self.assertEqual(lib2_result.org.short_name, 'org2') + + def test_authz_scope_individual_check_with_permission(self): + """ + Test that HasPermissionInContentLibraryScope.check() returns True + when authorization is granted. + + Given: + - Non-staff user + - Library exists + - Authorization system grants permission (mocked) + - NO legacy permissions + + Expected: + - check() returns True + """ + user = UserFactory.create(username="check_user", is_staff=False) + + with self.as_user(self.admin_user): + lib = self._create_library(slug="check-lib", org=self.org_short_name, title="Check Library") + + library_obj = ContentLibrary.objects.get_by_key(LibraryLocatorV2.from_string(lib["id"])) + + # CRITICAL: Ensure user has NO legacy permissions (test ONLY AuthZ) + ContentLibraryPermission.objects.filter(user=user).delete() + + with patch("openedx_authz.api.is_user_allowed", return_value=True): + result = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].check(user, library_obj) + + self.assertTrue(result, "Should return True when user is authorized") + + def test_authz_scope_individual_check_without_permission(self): + """ + Test that HasPermissionInContentLibraryScope.check() returns False + when authorization is denied. + + Given: + - Non-staff user + - Non-public library + - Authorization system denies permission (mocked) + - NO legacy permissions + + Expected: + - check() returns False + """ + user = UserFactory.create(username="no_perm_user", is_staff=False) + + with self.as_user(self.admin_user): + lib = self._create_library(slug="no-perm-lib", org=self.org_short_name, title="No Permission Library") + + library_obj = ContentLibrary.objects.get_by_key(LibraryLocatorV2.from_string(lib['id'])) + + # CRITICAL: Ensure user has NO legacy permissions (test ONLY AuthZ) + ContentLibraryPermission.objects.filter(user=user).delete() + + with patch('openedx_authz.api.is_user_allowed', return_value=False): + result = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].check(user, library_obj) + + self.assertFalse(result, "Should return False when user is not authorized") + + self.assertFalse(library_obj.allow_public_read) + self.assertFalse(user.is_staff) + + def test_authz_scope_handles_empty_scopes(self): + """ + Test that HasPermissionInContentLibraryScope.query() returns empty + result when user has no authorized scopes. + + Given: + - Non-staff user + - Library exists in database + - Authorization system returns empty scope list (mocked) + - NO legacy permissions + + Expected: + - Filter returns 0 libraries + - Library exists in database but is not accessible + """ + user = UserFactory.create(username="empty_user", is_staff=False) + + with self.as_user(self.admin_user): + self._create_library(slug="empty-lib", title="Empty Scopes Test") + + # CRITICAL: Ensure user has NO legacy permissions (test ONLY AuthZ) + ContentLibraryPermission.objects.filter(user=user).delete() + + with patch( + 'openedx_authz.api.get_scopes_for_user_and_permission', + return_value=[] + ): + filtered = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].filter( + user, + ContentLibrary.objects.filter(slug="empty-lib") + ).distinct() + + self.assertEqual( + filtered.count(), + 0, + "Should return 0 libraries when user has no authorized scopes", + ) + + self.assertTrue( + ContentLibrary.objects.filter(slug="empty-lib").exists(), + "Library should exist in database", + ) + + def test_authz_scope_q_object_has_correct_structure(self): + """ + Test that HasPermissionInContentLibraryScope.query() generates Q object + with structure: Q(org__short_name='X') & Q(slug='Y') for each scope. + + Multiple scopes should be OR'd: + (Q(org__short_name='org1') & Q(slug='lib1')) | (Q(org__short_name='org2') & Q(slug='lib2')) + + Note: This test focuses on Q object structure, not filtering behavior, + so legacy permissions don't affect the outcome. + """ + user = UserFactory.create(username="q_user") + rule = HasPermissionInContentLibraryScope(VIEW_LIBRARY, filter_keys=['org', 'slug']) + + with patch( + "openedx_authz.api.get_scopes_for_user_and_permission" + ) as mock_get_scopes: + # Create scopes with specific org/slug values we can verify + mock_scope1 = type("Scope", (), { + "library_key": type("Key", (), {"org": "specific-org1", "slug": "specific-slug1"})() + })() + mock_scope2 = type("Scope", (), { + "library_key": type("Key", (), {"org": "specific-org2", "slug": "specific-slug2"})() + })() + mock_get_scopes.return_value = [mock_scope1, mock_scope2] + + q_obj = rule.query(user) + + # Test 1: Verify it returns a Q object + self.assertIsInstance(q_obj, Q) + + # Test 2: Verify Q object uses OR connector (for multiple scopes) + self.assertEqual( + q_obj.connector, + 'OR', + "Should use OR to combine different library scopes", + ) + + # Test 3: Verify the Q object string contains the exact fields and values + q_str = str(q_obj) + + # Should filter by org__short_name field + self.assertIn( + "org__short_name", + q_str, + "Q object must filter by org__short_name field", + ) + + # Should filter by slug field + self.assertIn( + "slug", + q_str, + "Q object must filter by slug field", + ) + + # Should contain exact org values + self.assertIn( + "specific-org1", + q_str, + "Q object must include 'specific-org1'", + ) + self.assertIn( + "specific-org2", + q_str, + "Q object must include 'specific-org2'", + ) + + # Should contain exact slug values + self.assertIn( + "specific-slug1", + q_str, + "Q object must include 'specific-slug1'", + ) + self.assertIn( + 'specific-slug2', + q_str, + "Q object must include 'specific-slug2'", + ) + + def test_authz_scope_q_object_matches_exact_org_slug_pairs(self): + """ + Test that the Q object filters by EXACT (org, slug) pairs, not just org OR slug. + + Critical test: Verifies the rule generates: + Q(org__short_name='org1' AND slug='lib1') OR Q(org__short_name='org2' AND slug='lib2') + + NOT just: + Q(org__short_name IN ['org1', 'org2']) OR Q(slug IN ['lib1', 'lib2']) + + Creates scenario: + - lib1: org1 + lib1 (authorized) + - lib2: org2 + lib2 (authorized) + - lib3: org1 + lib3 (NOT authorized - same org, different slug) + - lib4: org3 + lib1 (NOT authorized - same slug, different org) + """ + user = UserFactory.create(username="exact_pair_user") + rule = HasPermissionInContentLibraryScope(VIEW_LIBRARY, filter_keys=['org', 'slug']) + + Organization.objects.get_or_create(short_name="pair-org1", defaults={"name": "Pair Org 1"}) + Organization.objects.get_or_create(short_name="pair-org2", defaults={"name": "Pair Org 2"}) + Organization.objects.get_or_create(short_name="pair-org3", defaults={"name": "Pair Org 3"}) + + with self.as_user(self.admin_user): + lib1 = self._create_library(slug="pair-lib1", org="pair-org1", title="Pair Lib 1") + lib2 = self._create_library(slug="pair-lib2", org="pair-org2", title="Pair Lib 2") + self._create_library(slug="pair-lib3", org="pair-org1", title="Pair Lib 3") # Same org as lib1 + self._create_library(slug="pair-lib1", org="pair-org3", title="Pair Lib 4") # Same slug as lib1 + + # CRITICAL: Ensure user has NO legacy permissions (test ONLY AuthZ filtering) + ContentLibraryPermission.objects.filter(user=user).delete() + + with patch( + 'openedx_authz.api.get_scopes_for_user_and_permission' + ) as mock_get_scopes: + # Authorize ONLY (pair-org1, pair-lib1) and (pair-org2, pair-lib2) + lib1_key = LibraryLocatorV2.from_string(lib1['id']) + lib2_key = LibraryLocatorV2.from_string(lib2['id']) + + mock_get_scopes.return_value = [ + type('Scope', (), {'library_key': lib1_key})(), + type('Scope', (), {'library_key': lib2_key})(), + ] + + q_obj = rule.query(user) + filtered = ContentLibrary.objects.filter(q_obj) + + # TEST: Verify EXACTLY 2 libraries match (lib1 and lib2 only) + self.assertEqual( + filtered.count(), + 2, + "Must match EXACTLY 2 libraries - only those with authorized (org, slug) pairs", + ) + + # TEST: Verify lib1 matches (pair-org1, pair-lib1) + lib1_result = filtered.filter(slug='pair-lib1', org__short_name='pair-org1') + self.assertEqual( + lib1_result.count(), + 1, + "Must match lib1: (pair-org1, pair-lib1) - this exact pair is authorized", + ) + + # TEST: Verify lib2 matches (pair-org2, pair-lib2) + lib2_result = filtered.filter(slug='pair-lib2', org__short_name='pair-org2') + self.assertEqual( + lib2_result.count(), + 1, + "Must match lib2: (pair-org2, pair-lib2) - this exact pair is authorized", + ) + + # TEST: Verify lib3 does NOT match (pair-org1, pair-lib3) + lib3_result = filtered.filter(slug='pair-lib3', org__short_name='pair-org1') + self.assertEqual( + lib3_result.count(), + 0, + "Must NOT match lib3: (pair-org1, pair-lib3) - only pair-lib1 is authorized for pair-org1", + ) + + # TEST: Verify lib4 does NOT match (pair-org3, pair-lib1) + lib4_result = filtered.filter(slug='pair-lib1', org__short_name='pair-org3') + self.assertEqual( + lib4_result.count(), + 0, + "Must NOT match lib4: (pair-org3, pair-lib1) - only pair-org1 is authorized for pair-lib1", + ) + + # TEST: Verify the result set contains exactly the right libraries + result_pairs = set(filtered.values_list('org__short_name', 'slug')) + expected_pairs = {('pair-org1', 'pair-lib1'), ('pair-org2', 'pair-lib2')} + self.assertEqual( + result_pairs, + expected_pairs, + f"Result must contain exactly {expected_pairs}, got {result_pairs}", + ) + + def test_authz_scope_with_combined_authz_and_legacy_permissions(self): + """ + Test that the filter returns libraries when user has BOTH AuthZ AND legacy permissions. + + The CAN_VIEW_THIS_CONTENT_LIBRARY permission uses OR logic: + is_user_active & ( + is_global_staff | + (allow_public_read & is_course_creator) | + HasPermissionInContentLibraryScope(VIEW_LIBRARY) | # AuthZ + has_explicit_read_permission_for_library # Legacy + ) + + This means a user with BOTH types of permissions should get access through EITHER system. + + Test scenario: + - lib1: User has AuthZ permission only + - lib2: User has legacy permission only + - lib3: User has BOTH AuthZ AND legacy permissions + - lib4: User has NO permissions + + Expected behavior: + - Filter returns lib1, lib2, and lib3 (NOT lib4) + - Having both permission types doesn't break filtering + - Each permission system contributes its authorized libraries + """ + user = UserFactory.create(username="combined_perm_user", is_staff=False) + + Organization.objects.get_or_create(short_name="comb-org", defaults={"name": "Combined Org"}) + + with self.as_user(self.admin_user): + lib1 = self._create_library(slug="comb-lib1", org="comb-org", title="AuthZ Only Library") + lib2 = self._create_library(slug="comb-lib2", org="comb-org", title="Legacy Only Library") + lib3 = self._create_library(slug="comb-lib3", org="comb-org", title="Both AuthZ and Legacy Library") + lib4 = self._create_library(slug="comb-lib4", org="comb-org", title="No Permissions Library") + + # Retrieve library objects for permission assignment + lib1_obj = ContentLibrary.objects.get_by_key(LibraryLocatorV2.from_string(lib1['id'])) + lib2_obj = ContentLibrary.objects.get_by_key(LibraryLocatorV2.from_string(lib2['id'])) + lib3_obj = ContentLibrary.objects.get_by_key(LibraryLocatorV2.from_string(lib3['id'])) + + # Set up legacy permissions: lib2 (legacy only), lib3 (both) + ContentLibraryPermission.objects.create( + library=lib2_obj, + user=user, + access_level=ContentLibraryPermission.READ_LEVEL, + ) + ContentLibraryPermission.objects.create( + library=lib3_obj, + user=user, + access_level=ContentLibraryPermission.READ_LEVEL, + ) + + with patch( + 'openedx_authz.api.get_scopes_for_user_and_permission' + ) as mock_get_scopes: + # Set up AuthZ permissions: lib1 (AuthZ only), lib3 (both) + lib1_key = LibraryLocatorV2.from_string(lib1['id']) + lib3_key = LibraryLocatorV2.from_string(lib3['id']) + + mock_get_scopes.return_value = [ + type('Scope', (), {'library_key': lib1_key})(), + type('Scope', (), {'library_key': lib3_key})(), + ] + + all_libs = ContentLibrary.objects.filter(slug__in=['comb-lib1', 'comb-lib2', 'comb-lib3', 'comb-lib4']) + filtered = perms[CAN_VIEW_THIS_CONTENT_LIBRARY].filter(user, all_libs).distinct() + + # TEST: Verify exactly 3 libraries returned (lib1, lib2, lib3 - NOT lib4) + self.assertEqual( + filtered.count(), + 3, + "Should return exactly 3 libraries: AuthZ-only, legacy-only, and both", + ) + + # TEST: Verify correct libraries are included + slugs = set(filtered.values_list('slug', flat=True)) + self.assertIn('comb-lib1', slugs, "lib1 should be accessible via AuthZ permission") + self.assertIn('comb-lib2', slugs, "lib2 should be accessible via legacy permission") + self.assertIn('comb-lib3', slugs, "lib3 should be accessible via BOTH AuthZ and legacy permissions") + self.assertNotIn('comb-lib4', slugs, "lib4 should NOT be accessible (no permissions)") + + # TEST: Verify lib3 doesn't get duplicated despite having both permission types + lib3_results = filtered.filter(slug='comb-lib3') + self.assertEqual( + lib3_results.count(), + 1, + "lib3 should appear exactly once despite having both AuthZ and legacy permissions", + ) + + # TEST: Verify the permission sources work independently + # This demonstrates the OR logic: user gets access if EITHER permission type grants it + result_pairs = set(filtered.values_list('org__short_name', 'slug')) + expected_pairs = { + ('comb-org', 'comb-lib1'), # AuthZ only + ('comb-org', 'comb-lib2'), # Legacy only + ('comb-org', 'comb-lib3'), # Both + } + self.assertEqual( + result_pairs, + expected_pairs, + f"Should get exactly the 3 authorized libraries via OR logic, got {result_pairs}", + ) + + @ddt.ddt class ContentLibraryXBlockValidationTest(APITestCase): """Tests only focused on service validation, no Learning Core interactions here.""" @@ -1244,3 +1708,282 @@ def test_xblock_handler_invalid_key(self): secure_token='random', ))) self.assertEqual(response.status_code, 404) + + +@skip_unless_cms +class ContentLibrariesRestAPIAuthzIntegrationTestCase(ContentLibrariesRestApiTest): + """ + Test that Content Libraries REST API endpoints respect AuthZ roles and permissions. + + Roles tested: + 1. Library Admin: Full access to all library operations. + 2. Library Author: Can view and edit library content, but cannot delete the library. + 3. Library Contributor: Can view and edit library content, but cannot delete or publish the library. + 4. Library User: Can only view library content. + """ + + def setUp(self): + super().setUp() + self._seed_database_with_policies() + + self.library_admin = UserFactory.create( + username="library_admin", + email="libadmin@example.com") + self.library_author = UserFactory.create( + username="library_author", + email="libauthor@example.com") + self.library_contributor = UserFactory.create( + username="library_contributor", + email="libcontributor@example.com") + self.library_user = UserFactory.create( + username="library_user", + email="libuser@example.com") + self.random_user = UserFactory.create( + username="random_user", + email="random@example.com") + + # Define user groups by permission level + self.list_of_all_users = [ + self.library_admin, + self.library_author, + self.library_contributor, + self.library_user, + self.random_user, + ] + self.library_viewers = [self.library_admin, self.library_author, self.library_contributor, self.library_user] + self.library_editors = [self.library_admin, self.library_author, self.library_contributor] + self.library_publishers = [self.library_admin, self.library_author] + self.library_collection_editors = [self.library_admin, self.library_author, self.library_contributor] + self.library_deleters = [self.library_admin] + + # Create library and assign roles + library = self._create_library( + slug="authzlib", + title="AuthZ Test Library", + description="Testing AuthZ", + ) + self.lib_id = library["id"] + + authz_api.assign_role_to_user_in_scope( + self.library_admin.username, + roles.LIBRARY_ADMIN.external_key, self.lib_id) + authz_api.assign_role_to_user_in_scope( + self.library_author.username, + roles.LIBRARY_AUTHOR.external_key, self.lib_id) + authz_api.assign_role_to_user_in_scope( + self.library_contributor.username, + roles.LIBRARY_CONTRIBUTOR.external_key, self.lib_id) + authz_api.assign_role_to_user_in_scope( + self.library_user.username, + roles.LIBRARY_USER.external_key, self.lib_id) + AuthzEnforcer.get_enforcer().load_policy() # Load policies to simulate fresh start + + def tearDown(self): + """Clean up after each test to ensure isolation.""" + super().tearDown() + AuthzEnforcer.get_enforcer().clear_policy() # Clear policies after each test to ensure isolation + + @classmethod + def _seed_database_with_policies(cls): + """Seed the database with policies from the policy file. + + This simulates the one-time database seeding that would happen + during application deployment, separate from the runtime policy loading. + """ + import pkg_resources + from openedx_authz.engine.utils import migrate_policy_between_enforcers + import casbin + + global_enforcer = AuthzEnforcer.get_enforcer() + global_enforcer.load_policy() + model_path = pkg_resources.resource_filename("openedx_authz.engine", "config/model.conf") + policy_path = pkg_resources.resource_filename("openedx_authz.engine", "config/authz.policy") + + migrate_policy_between_enforcers( + source_enforcer=casbin.Enforcer(model_path, policy_path), + target_enforcer=global_enforcer, + ) + global_enforcer.clear_policy() # Clear to simulate fresh start for each test + + def _all_users_excluding(self, excluded_users): + return set(self.list_of_all_users) - set(excluded_users) + + def test_view_permissions(self): + """ + Verify that only users with view permissions can view. + """ + # Test library view access + for user in self.library_viewers: + with self.as_user(user): + self._get_library(self.lib_id, expect_response=status.HTTP_200_OK) + for user in self._all_users_excluding(self.library_viewers): + with self.as_user(user): + self._get_library(self.lib_id, expect_response=status.HTTP_403_FORBIDDEN) + + def test_edit_permissions(self): + """ + Verify that only users with edit permissions can edit. + """ + # Test library edit access + for user in self.library_editors: + with self.as_user(user): + self._update_library( + self.lib_id, + description=f"Description by {user.username}", + expect_response=status.HTTP_200_OK, + ) + #Verify the permitted changes were made + data = self._get_library(self.lib_id) + assert data['description'] == f"Description by {user.username}" + + for user in self._all_users_excluding(self.library_editors): + with self.as_user(user): + self._update_library( + self.lib_id, + description="I can't edit this.", expect_response=status.HTTP_403_FORBIDDEN) + + # Verify the no permitted changes weren't made: + data = self._get_library(self.lib_id) + assert data['description'] != "I can't edit this." + + # Library XBlock editing + for user in self.library_editors: + with self.as_user(user): + # They can create blocks + block_data = self._add_block_to_library(self.lib_id, "problem", f"problem_{user.username}") + # They can modify blocks + self._set_library_block_olx( + block_data["id"], + "", + expect_response=status.HTTP_200_OK) + self._set_library_block_fields( + block_data["id"], + {"data": "", "metadata": {}}, + expect_response=status.HTTP_200_OK) + self._set_library_block_asset( + block_data["id"], + "static/test.txt", + b"data", + expect_response=status.HTTP_200_OK) + # They can remove blocks + self._delete_library_block(block_data["id"], expect_response=status.HTTP_200_OK) + # Verify deletion + self._get_library_block(block_data["id"], expect_response=404) + + # Recreate blocks for further tests + block_data = self._add_block_to_library(self.lib_id, "problem", "new_problem") + + for user in self._all_users_excluding(self.library_editors): + with self.as_user(user): + self._add_block_to_library( + self.lib_id, + "problem", + "problem1", + expect_response=status.HTTP_403_FORBIDDEN) + # They can't modify blocks + self._set_library_block_olx( + block_data["id"], + "", + expect_response=status.HTTP_403_FORBIDDEN) + self._set_library_block_fields( + block_data["id"], + {"data": "", "metadata": {}}, + expect_response=status.HTTP_403_FORBIDDEN) + self._set_library_block_asset( + block_data["id"], + "static/test.txt", + b"data", + expect_response=status.HTTP_403_FORBIDDEN) + # They can't remove blocks + self._delete_library_block(block_data["id"], expect_response=status.HTTP_403_FORBIDDEN) + + def test_publish_permissions(self): + """ + Verify that only users with publish permissions can publish. + """ + # Test publish access + for user in self.library_publishers: + with self.as_user(user): + block_data = self._add_block_to_library(self.lib_id, "problem", f"problem_{user.username}_1") + self._publish_library_block(block_data["id"], expect_response=status.HTTP_200_OK) + block_data = self._add_block_to_library(self.lib_id, "problem", f"problem_{user.username}_2") + assert self._get_library(self.lib_id)['has_unpublished_changes'] is True + self._commit_library_changes(self.lib_id, expect_response=status.HTTP_200_OK) + assert self._get_library(self.lib_id)['has_unpublished_changes'] is False + + block_data = self._add_block_to_library(self.lib_id, "problem", "draft_problem") + assert self._get_library(self.lib_id)['has_unpublished_changes'] is True + + for user in self._all_users_excluding(self.library_publishers): + with self.as_user(user): + self._publish_library_block(block_data["id"], expect_response=status.HTTP_403_FORBIDDEN) + self._commit_library_changes(self.lib_id, expect_response=status.HTTP_403_FORBIDDEN) + # Verify that no changes were published + assert self._get_library(self.lib_id)['has_unpublished_changes'] is True + + def test_collection_permissions(self): + """ + Verify that only users with collection permissions can perform collection actions. + """ + library_key = LibraryLocatorV2.from_string(self.lib_id) + block_data = self._add_block_to_library(self.lib_id, "problem", "collection_problem") + # Test library collection access + for user in self.library_collection_editors: + with self.as_user(user): + # Create collection + collection_data = self._create_collection( + self.lib_id, + title=f"Temp Collection {user.username}", + expect_response=status.HTTP_200_OK) + collection_id = collection_data["key"] + collection_key = LibraryCollectionLocator(lib_key=library_key, collection_id=collection_id) + # Update collection + self._update_collection(collection_key, title="Updated Collection", expect_response=status.HTTP_200_OK) + self._add_items_to_collection( + collection_key, + item_keys=[block_data["id"]], + expect_response=status.HTTP_200_OK) + # Delete collection + self._soft_delete_collection(collection_key, expect_response=status.HTTP_204_NO_CONTENT) + + collection_data = self._create_collection( + self.lib_id, + title="New Temp Collection", + expect_response=status.HTTP_200_OK) + collection_id = collection_data["key"] + collection_key = LibraryCollectionLocator(lib_key=library_key, collection_id=collection_id) + + for user in self._all_users_excluding(self.library_collection_editors): + with self.as_user(user): + # Attempt to create collection + self._create_collection( + self.lib_id, + title="Unauthorized Collection", + expect_response=status.HTTP_403_FORBIDDEN) + # Attempt to update collection + self._update_collection( + collection_key, + title="Unauthorized Change", + expect_response=status.HTTP_403_FORBIDDEN) + self._add_items_to_collection( + collection_key, + item_keys=[block_data["id"]], + expect_response=status.HTTP_403_FORBIDDEN) + # Attempt to delete collection + self._soft_delete_collection(collection_key, expect_response=status.HTTP_403_FORBIDDEN) + + def test_delete_library_permissions(self): + """ + Verify that only users with delete permissions can delete a library. + """ + # Test library delete access + for user in self._all_users_excluding(self.library_deleters): + with self.as_user(user): + result = self._delete_library(self.lib_id, expect_response=status.HTTP_403_FORBIDDEN) + assert 'detail' in result # Error message + assert 'permission' in result['detail'].lower() + + for user in self.library_deleters: + with self.as_user(user): + result = self._delete_library(self.lib_id, expect_response=status.HTTP_200_OK) + assert result == {} diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index 6ba1049082d1..3b36df3f4075 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -514,12 +514,12 @@ def test_create_taxonomy(self, user_attr: str, expected_status: int) -> None: @ddt.data( ('staff', 11), - ("content_creatorA", 17), - ("library_staffA", 17), - ("library_userA", 17), - ("instructorA", 17), - ("course_instructorA", 17), - ("course_staffA", 17), + ("content_creatorA", 22), + ("library_staffA", 22), + ("library_userA", 22), + ("instructorA", 22), + ("course_instructorA", 22), + ("course_staffA", 22), ) @ddt.unpack def test_list_taxonomy_query_count(self, user_attr: str, expected_queries: int): @@ -1927,16 +1927,16 @@ def test_get_copied_tags(self): ('staff', 'courseA', 8), ('staff', 'libraryA', 8), ('staff', 'collection_key', 8), - ("content_creatorA", 'courseA', 12, False), - ("content_creatorA", 'libraryA', 12, False), - ("content_creatorA", 'collection_key', 12, False), - ("library_staffA", 'libraryA', 12, False), # Library users can only view objecttags, not change them? - ("library_staffA", 'collection_key', 12, False), - ("library_userA", 'libraryA', 12, False), - ("library_userA", 'collection_key', 12, False), - ("instructorA", 'courseA', 12), - ("course_instructorA", 'courseA', 12), - ("course_staffA", 'courseA', 12), + ("content_creatorA", 'courseA', 17, False), + ("content_creatorA", 'libraryA', 17, False), + ("content_creatorA", 'collection_key', 17, False), + ("library_staffA", 'libraryA', 17, False), # Library users can only view objecttags, not change them? + ("library_staffA", 'collection_key', 17, False), + ("library_userA", 'libraryA', 17, False), + ("library_userA", 'collection_key', 17, False), + ("instructorA", 'courseA', 17), + ("course_instructorA", 'courseA', 17), + ("course_staffA", 'courseA', 17), ) @ddt.unpack def test_object_tags_query_count( diff --git a/requirements/common_constraints.txt b/requirements/common_constraints.txt index c13c406e6176..1f3e81f50334 100644 --- a/requirements/common_constraints.txt +++ b/requirements/common_constraints.txt @@ -16,7 +16,7 @@ # this file from Github directly. It does not require packaging in edx-lint. # using LTS django version - +Django<6.0 # elasticsearch>=7.14.0 includes breaking changes in it which caused issues in discovery upgrade process. # elastic search changelog: https://www.elastic.co/guide/en/enterprise-search/master/release-notes-7.14.0.html diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index f4fa71e700fb..f3387eb7bb18 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -172,6 +172,7 @@ defusedxml==0.7.1 # social-auth-core django==5.2.7 # via + # -c requirements/common_constraints.txt # -c requirements/constraints.txt # -r requirements/edx/kernel.in # casbin-django-orm-adapter @@ -825,7 +826,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==0.11.2 +openedx-authz==0.19.0 # via -r requirements/edx/kernel.in openedx-calc==4.0.2 # via -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 3afded2e283b..6e75615ec812 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -345,6 +345,7 @@ distlib==0.4.0 # virtualenv django==5.2.7 # via + # -c requirements/common_constraints.txt # -c requirements/constraints.txt # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt @@ -1375,7 +1376,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==0.11.2 +openedx-authz==0.19.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index e25735f3961c..5e465e8905ba 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -234,6 +234,7 @@ defusedxml==0.7.1 # social-auth-core django==5.2.7 # via + # -c requirements/common_constraints.txt # -c requirements/constraints.txt # -r requirements/edx/base.txt # casbin-django-orm-adapter @@ -1002,7 +1003,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==0.11.2 +openedx-authz==0.19.0 # via -r requirements/edx/base.txt openedx-calc==4.0.2 # via -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index d0faa2471265..08f7639ace60 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -261,6 +261,7 @@ distlib==0.4.0 # via virtualenv django==5.2.7 # via + # -c requirements/common_constraints.txt # -c requirements/constraints.txt # -r requirements/edx/base.txt # casbin-django-orm-adapter @@ -1048,7 +1049,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==0.11.2 +openedx-authz==0.19.0 # via -r requirements/edx/base.txt openedx-calc==4.0.2 # via -r requirements/edx/base.txt diff --git a/scripts/user_retirement/requirements/base.txt b/scripts/user_retirement/requirements/base.txt index a14836ff5e95..b726b8a49f40 100644 --- a/scripts/user_retirement/requirements/base.txt +++ b/scripts/user_retirement/requirements/base.txt @@ -36,6 +36,7 @@ cryptography==45.0.7 # pyjwt django==5.2.7 # via + # -c requirements/common_constraints.txt # -c requirements/constraints.txt # django-crum # django-waffle From 699c831fa397eb8d1fc69a08b24219cef447723b Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 24 Nov 2025 00:03:34 -0500 Subject: [PATCH 08/21] fix: restrict forum bulk delete to global staff This was originally permitted for forum moderators and course staff as a way to fight spam, but it was decided that this functionality was too dangerous to open up that widely. There are some tentative plans around how to make this a more fully supported feature, but until then, we're restricting this to global staff on the Ulmo release branch as an interim measure. The frontend was already disabled in the Ulmo release, meaning that this will be a backend-only API (i.e. if you really know what you're doing and absolutely need this functionality). The assumption is that this feature will continue to be developed on the master branch and will be in better shape for Verawood. --- .../discussion/rest_api/permissions.py | 25 +++---------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/lms/djangoapps/discussion/rest_api/permissions.py b/lms/djangoapps/discussion/rest_api/permissions.py index cfcea5b32834..a1ae60c35ef3 100644 --- a/lms/djangoapps/discussion/rest_api/permissions.py +++ b/lms/djangoapps/discussion/rest_api/permissions.py @@ -6,7 +6,7 @@ from opaque_keys.edx.keys import CourseKey from rest_framework import permissions -from common.djangoapps.student.models import CourseAccessRole, CourseEnrollment +from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.student.roles import ( CourseInstructorRole, CourseStaffRole, @@ -19,7 +19,7 @@ from openedx.core.djangoapps.django_comment_common.comment_client.comment import Comment from openedx.core.djangoapps.django_comment_common.comment_client.thread import Thread from openedx.core.djangoapps.django_comment_common.models import ( - Role, FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_MODERATOR + FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_MODERATOR ) @@ -194,26 +194,7 @@ def can_take_action_on_spam(user, course_id): user: User object course_id: CourseKey or string of course_id """ - if GlobalStaff().has_user(user): - return True - - if isinstance(course_id, str): - course_id = CourseKey.from_string(course_id) - org_id = course_id.org - course_ids = CourseEnrollment.objects.filter(user=user).values_list('course_id', flat=True) - course_ids = [c_id for c_id in course_ids if c_id.org == org_id] - user_roles = set( - Role.objects.filter( - users=user, - course_id__in=course_ids, - ).values_list('name', flat=True).distinct() - ) - if bool(user_roles & {FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR}): - return True - - if CourseAccessRole.objects.filter(user=user, course_id__in=course_ids, role__in=["instructor", "staff"]).exists(): - return True - return False + return GlobalStaff().has_user(user) class IsAllowedToBulkDelete(permissions.BasePermission): From 97ccbc79a402b061d8793dd1c078d01014dc4d60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Chris=20Ch=C3=A1vez?= Date: Tue, 25 Nov 2025 16:17:35 -0500 Subject: [PATCH 09/21] fix: Publish components/container in legacy libraries migration (#37644) (#37679) - Fix the issue described in https://github.com/openedx/frontend-app-authoring/issues/2626 - Publish components and containers after migrate (cherry picked from commit b9e5683b670a3a1ec5c8e92559feb5c32684e675) --- cms/djangoapps/modulestore_migrator/tasks.py | 20 ++++++++++++++++++- .../modulestore_migrator/tests/test_tasks.py | 5 +++++ .../content_libraries/api/blocks.py | 4 ++-- .../content_libraries/api/containers.py | 11 ++++++++-- .../content_libraries/rest_api/blocks.py | 2 +- 5 files changed, 36 insertions(+), 6 deletions(-) diff --git a/cms/djangoapps/modulestore_migrator/tasks.py b/cms/djangoapps/modulestore_migrator/tasks.py index 040501185339..ef2386c69d34 100644 --- a/cms/djangoapps/modulestore_migrator/tasks.py +++ b/cms/djangoapps/modulestore_migrator/tasks.py @@ -1077,7 +1077,8 @@ def _migrate_container( entity_id=container.container_pk, version_num=container.draft_version_num, ) - return authoring_api.create_next_container_version( + + container_publishable_entity_version = authoring_api.create_next_container_version( container.container_pk, title=title, entity_rows=[ @@ -1089,6 +1090,17 @@ def _migrate_container( container_version_cls=container_type.container_model_classes[1], ).publishable_entity_version + # Publish the container + # Call post publish events synchronously to avoid + # an error when calling `wait_for_post_publish_events` + # inside a celery task. + libraries_api.publish_container_changes( + container.container_key, + context.created_by, + call_post_publish_events_sync=True, + ) + return container_publishable_entity_version + def _migrate_component( *, @@ -1153,6 +1165,12 @@ def _migrate_component( authoring_api.create_component_version_content( component_version.pk, content_pk, key=new_path ) + + # Publish the component + libraries_api.publish_component_changes( + libraries_api.library_component_usage_key(context.target_library_key, component), + context.created_by, + ) return component_version.publishable_entity_version diff --git a/cms/djangoapps/modulestore_migrator/tests/test_tasks.py b/cms/djangoapps/modulestore_migrator/tests/test_tasks.py index afd422bf04ef..244d435de50a 100644 --- a/cms/djangoapps/modulestore_migrator/tests/test_tasks.py +++ b/cms/djangoapps/modulestore_migrator/tests/test_tasks.py @@ -440,6 +440,9 @@ def test_migrate_component_success(self): "problem", result.componentversion.component.component_type.name ) + # The component is published + self.assertFalse(result.componentversion.component.versioning.has_unpublished_changes) + def test_migrate_component_with_static_content(self): """ Test _migrate_component with static file content @@ -897,6 +900,8 @@ def test_migrate_container_different_container_types(self): container_version = result.containerversion self.assertEqual(container_version.title, f"Test {block_type.title()}") + # The container is published + self.assertFalse(authoring_api.contains_unpublished_changes(container_version.container.pk)) def test_migrate_container_replace_existing_false(self): """ diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index af44d1874508..be5b296147fd 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -956,7 +956,7 @@ def delete_library_block_static_asset_file(usage_key, file_path, user=None): ) -def publish_component_changes(usage_key: LibraryUsageLocatorV2, user: UserType): +def publish_component_changes(usage_key: LibraryUsageLocatorV2, user_id: int): """ Publish all pending changes in a single component. """ @@ -969,7 +969,7 @@ def publish_component_changes(usage_key: LibraryUsageLocatorV2, user: UserType): drafts_to_publish = authoring_api.get_all_drafts(learning_package.id).filter(entity__key=component.key) # Publish the component and update anything that needs to be updated (e.g. search index): publish_log = authoring_api.publish_from_drafts( - learning_package.id, draft_qset=drafts_to_publish, published_by=user.id, + learning_package.id, draft_qset=drafts_to_publish, published_by=user_id, ) # Since this is a single component, it should be safe to process synchronously and in-process: tasks.send_events_after_publish(publish_log.pk, str(library_key)) diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 28f05a05723a..5cca0b3a994b 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -575,7 +575,11 @@ def get_containers_contains_item( ] -def publish_container_changes(container_key: LibraryContainerLocator, user_id: int | None) -> None: +def publish_container_changes( + container_key: LibraryContainerLocator, + user_id: int | None, + call_post_publish_events_sync=False, +) -> None: """ [ 🛑 UNSTABLE ] Publish all unpublished changes in a container and all its child containers/blocks. @@ -595,7 +599,10 @@ def publish_container_changes(container_key: LibraryContainerLocator, user_id: i ) # Update the search index (and anything else) for the affected container + blocks # This is mostly synchronous but may complete some work asynchronously if there are a lot of changes. - tasks.wait_for_post_publish_events(publish_log, library_key) + if call_post_publish_events_sync: + tasks.send_events_after_publish(publish_log.pk, str(library_key)) + else: + tasks.wait_for_post_publish_events(publish_log, library_key) def copy_container(container_key: LibraryContainerLocator, user_id: int) -> UserClipboardData: diff --git a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py index 7aa15f6e7834..e72980f6ba0d 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/blocks.py @@ -241,7 +241,7 @@ def post(self, request, usage_key_str): request.user, authz_permissions.PUBLISH_LIBRARY_CONTENT ) - api.publish_component_changes(key, request.user) + api.publish_component_changes(key, request.user.id) return Response({}) From d7665ba3dcfe869e8ea3b94466b6dd02b80ca03b Mon Sep 17 00:00:00 2001 From: Taimoor Ahmed <68893403+taimoor-ahmed-1@users.noreply.github.com> Date: Wed, 26 Nov 2025 13:42:39 +0500 Subject: [PATCH 10/21] fix: send thread_created signal after transaction commit (#37675) (#37688) Prevents notification failures with MySQL backend by ensuring signals are only sent after database transactions commit. This fixes race conditions where Celery workers couldn't see newly created threads. - Added send_signal_after_commit() helper function - Updated both thread creation paths to use the helper Co-authored-by: Taimoor Ahmed --- .../django_comment_client/base/tests_v2.py | 13 ++++- .../django_comment_client/base/views.py | 58 ++++++++++++++----- lms/djangoapps/discussion/rest_api/api.py | 46 +++++++++++---- .../discussion/rest_api/tests/test_api_v2.py | 50 +++++++++------- lms/djangoapps/discussion/rest_api/utils.py | 24 +++++++- 5 files changed, 142 insertions(+), 49 deletions(-) diff --git a/lms/djangoapps/discussion/django_comment_client/base/tests_v2.py b/lms/djangoapps/discussion/django_comment_client/base/tests_v2.py index 7bc84e5038c0..1f5ae7805740 100644 --- a/lms/djangoapps/discussion/django_comment_client/base/tests_v2.py +++ b/lms/djangoapps/discussion/django_comment_client/base/tests_v2.py @@ -180,7 +180,8 @@ def test_flag(self): with mock.patch( "openedx.core.djangoapps.django_comment_common.signals.thread_flagged.send" ) as signal_mock: - response = self.call_view("flag_abuse_for_thread", "update_thread_flag") + with self.captureOnCommitCallbacks(execute=True): + response = self.call_view("flag_abuse_for_thread", "update_thread_flag") self._assert_json_response_contains_group_info(response) self.assertEqual(signal_mock.call_count, 1) response = self.call_view("un_flag_abuse_for_thread", "update_thread_flag") @@ -471,10 +472,15 @@ def setUp(self): def assert_discussion_signals(self, signal, user=None): if user is None: user = self.student + # Use captureOnCommitCallbacks to execute on_commit callbacks during tests, + # since signals are now deferred until after transaction commit. + # Order matters: assert_signal_sent must be outer context so the signal + # fires (via captureOnCommitCallbacks) before the assertion check. with self.assert_signal_sent( views, signal, sender=None, user=user, exclude_args=("post",) ): - yield + with self.captureOnCommitCallbacks(execute=True): + yield def test_create_thread(self): with self.assert_discussion_signals("thread_created"): @@ -1218,7 +1224,8 @@ def test_flag(self): with mock.patch( "openedx.core.djangoapps.django_comment_common.signals.comment_flagged.send" ) as signal_mock: - self.call_view("flag_abuse_for_comment", "update_comment_flag") + with self.captureOnCommitCallbacks(execute=True): + self.call_view("flag_abuse_for_comment", "update_comment_flag") self.assertEqual(signal_mock.call_count, 1) diff --git a/lms/djangoapps/discussion/django_comment_client/base/views.py b/lms/djangoapps/discussion/django_comment_client/base/views.py index 95d5a020108f..14ce9c4b575a 100644 --- a/lms/djangoapps/discussion/django_comment_client/base/views.py +++ b/lms/djangoapps/discussion/django_comment_client/base/views.py @@ -50,6 +50,7 @@ prepare_content, sanitize_body ) +from lms.djangoapps.discussion.rest_api.utils import send_signal_after_commit from openedx.core.djangoapps.django_comment_common.signals import ( comment_created, comment_deleted, @@ -587,7 +588,10 @@ def create_thread(request, course_id, commentable_id): thread.save() - thread_created.send(sender=None, user=user, post=thread) + # Use send_signal_after_commit() to ensure the signal is sent only after the transaction commits. + send_signal_after_commit( + lambda: thread_created.send(sender=None, user=user, post=thread) + ) # patch for backward compatibility to comments service if 'pinned' not in thread.attributes: @@ -598,7 +602,9 @@ def create_thread(request, course_id, commentable_id): if follow: cc_user = cc.User.from_django_user(user) cc_user.follow(thread, course_id) - thread_followed.send(sender=None, user=user, post=thread) + send_signal_after_commit( + lambda: thread_followed.send(sender=None, user=user, post=thread) + ) data = thread.to_dict() @@ -645,7 +651,9 @@ def update_thread(request, course_id, thread_id): thread.save() - thread_edited.send(sender=None, user=user, post=thread) + send_signal_after_commit( + lambda: thread_edited.send(sender=None, user=user, post=thread) + ) track_thread_edited_event(request, course, thread, None) if request.headers.get('x-requested-with') == 'XMLHttpRequest': @@ -688,7 +696,9 @@ def _create_comment(request, course_key, thread_id=None, parent_id=None): ) comment.save(params={"course_id": str(course_key)}) - comment_created.send(sender=None, user=user, post=comment) + send_signal_after_commit( + lambda: comment_created.send(sender=None, user=user, post=comment) + ) followed = post.get('auto_subscribe', 'false').lower() == 'true' @@ -729,7 +739,9 @@ def delete_thread(request, course_id, thread_id): course = get_course_with_access(request.user, 'load', course_key) thread = cc.Thread.find(thread_id) thread.delete(course_id=course_id) - thread_deleted.send(sender=None, user=request.user, post=thread) + send_signal_after_commit( + lambda: thread_deleted.send(sender=None, user=request.user, post=thread) + ) track_thread_deleted_event(request, course, thread) return JsonResponse(prepare_content(thread.to_dict(), course_key)) @@ -751,7 +763,9 @@ def update_comment(request, course_id, comment_id): comment.body = sanitize_body(request.POST["body"]) comment.save(params={"course_id": course_id}) - comment_edited.send(sender=None, user=request.user, post=comment) + send_signal_after_commit( + lambda: comment_edited.send(sender=None, user=request.user, post=comment) + ) track_comment_edited_event(request, course, comment, None) if request.headers.get('x-requested-with') == 'XMLHttpRequest': @@ -776,7 +790,9 @@ def endorse_comment(request, course_id, comment_id): comment.endorsed = endorsed comment.endorsement_user_id = user.id comment.save(params={"course_id": course_id}) - comment_endorsed.send(sender=None, user=user, post=comment) + send_signal_after_commit( + lambda: comment_endorsed.send(sender=None, user=user, post=comment) + ) track_forum_response_mark_event(request, course, comment, endorsed) return JsonResponse(prepare_content(comment.to_dict(), course_key)) @@ -828,7 +844,9 @@ def delete_comment(request, course_id, comment_id): course = get_course_with_access(request.user, 'load', course_key) comment = cc.Comment.find(comment_id) comment.delete(course_id=course_id) - comment_deleted.send(sender=None, user=request.user, post=comment) + send_signal_after_commit( + lambda: comment_deleted.send(sender=None, user=request.user, post=comment) + ) track_comment_deleted_event(request, course, comment) return JsonResponse(prepare_content(comment.to_dict(), course_key)) @@ -847,7 +865,9 @@ def _vote_or_unvote(request, course_id, obj, value='up', undo_vote=False): # (People could theoretically downvote by handcrafting AJAX requests.) else: user.vote(obj, value, course_id) - thread_voted.send(sender=None, user=request.user, post=obj) + send_signal_after_commit( + lambda: thread_voted.send(sender=None, user=request.user, post=obj) + ) track_voted_event(request, course, obj, value, undo_vote) return JsonResponse(prepare_content(obj.to_dict(), course_key)) @@ -861,7 +881,9 @@ def vote_for_comment(request, course_id, comment_id, value): """ comment = cc.Comment.find(comment_id) result = _vote_or_unvote(request, course_id, comment, value) - comment_voted.send(sender=None, user=request.user, post=comment) + send_signal_after_commit( + lambda: comment_voted.send(sender=None, user=request.user, post=comment) + ) return result @@ -914,7 +936,9 @@ def flag_abuse_for_thread(request, course_id, thread_id): thread = cc.Thread.find(thread_id) thread.flagAbuse(user, thread, course_id) track_discussion_reported_event(request, course, thread) - thread_flagged.send(sender='flag_abuse_for_thread', user=request.user, post=thread) + send_signal_after_commit( + lambda: thread_flagged.send(sender='flag_abuse_for_thread', user=request.user, post=thread) + ) return JsonResponse(prepare_content(thread.to_dict(), course_key)) @@ -953,7 +977,9 @@ def flag_abuse_for_comment(request, course_id, comment_id): comment = cc.Comment.find(comment_id) comment.flagAbuse(user, comment, course_id) track_discussion_reported_event(request, course, comment) - comment_flagged.send(sender='flag_abuse_for_comment', user=request.user, post=comment) + send_signal_after_commit( + lambda: comment_flagged.send(sender='flag_abuse_for_comment', user=request.user, post=comment) + ) return JsonResponse(prepare_content(comment.to_dict(), course_key)) @@ -1019,7 +1045,9 @@ def follow_thread(request, course_id, thread_id): # lint-amnesty, pylint: disab course = get_course_by_id(course_key) thread = cc.Thread.find(thread_id) user.follow(thread, course_id=course_id) - thread_followed.send(sender=None, user=request.user, post=thread) + send_signal_after_commit( + lambda: thread_followed.send(sender=None, user=request.user, post=thread) + ) track_thread_followed_event(request, course, thread, True) return JsonResponse({}) @@ -1051,7 +1079,9 @@ def unfollow_thread(request, course_id, thread_id): # lint-amnesty, pylint: dis user = cc.User.from_django_user(request.user) thread = cc.Thread.find(thread_id) user.unfollow(thread, course_id=course_id) - thread_unfollowed.send(sender=None, user=request.user, post=thread) + send_signal_after_commit( + lambda: thread_unfollowed.send(sender=None, user=request.user, post=thread) + ) track_thread_followed_event(request, course, thread, False) return JsonResponse({}) diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index b87852c16cfa..1c0b05735208 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -128,6 +128,7 @@ discussion_open_for_user, get_usernames_for_course, get_usernames_from_search_string, + send_signal_after_commit, set_attribute, is_posting_allowed, can_user_notify_all_learners, is_captcha_enabled, get_captcha_site_key_by_platform @@ -1382,7 +1383,9 @@ def _handle_following_field(form_value, user, cc_content, request): else: user.unfollow(cc_content) signal = thread_followed if form_value else thread_unfollowed - signal.send(sender=None, user=user, post=cc_content) + send_signal_after_commit( + lambda: signal.send(sender=None, user=user, post=cc_content) + ) track_thread_followed_event(request, course, cc_content, form_value) @@ -1395,9 +1398,13 @@ def _handle_abuse_flagged_field(form_value, user, cc_content, request): track_discussion_reported_event(request, course, cc_content) if ENABLE_DISCUSSIONS_MFE.is_enabled(course_key): if cc_content.type == 'thread': - thread_flagged.send(sender='flag_abuse_for_thread', user=user, post=cc_content) + send_signal_after_commit( + lambda: thread_flagged.send(sender='flag_abuse_for_thread', user=user, post=cc_content) + ) else: - comment_flagged.send(sender='flag_abuse_for_comment', user=user, post=cc_content) + send_signal_after_commit( + lambda: comment_flagged.send(sender='flag_abuse_for_comment', user=user, post=cc_content) + ) else: remove_all = bool(is_privileged_user(course_key, User.objects.get(id=user.id))) cc_content.unFlagAbuse(user, cc_content, remove_all) @@ -1407,7 +1414,9 @@ def _handle_abuse_flagged_field(form_value, user, cc_content, request): def _handle_voted_field(form_value, cc_content, api_content, request, context): """vote or undo vote on thread/comment""" signal = thread_voted if cc_content.type == 'thread' else comment_voted - signal.send(sender=None, user=context["request"].user, post=cc_content) + send_signal_after_commit( + lambda: signal.send(sender=None, user=context["request"].user, post=cc_content) + ) if form_value: context["cc_requester"].vote(cc_content, "up") api_content["vote_count"] += 1 @@ -1452,7 +1461,9 @@ def _handle_comment_signals(update_data, comment, user, sender=None): """ for key, value in update_data.items(): if key == "endorsed" and value is True: - comment_endorsed.send(sender=sender, user=user, post=comment) + send_signal_after_commit( + lambda: comment_endorsed.send(sender=sender, user=user, post=comment) + ) def create_thread(request, thread_data): @@ -1502,7 +1513,10 @@ def create_thread(request, thread_data): raise ValidationError(dict(list(serializer.errors.items()) + list(actions_form.errors.items()))) serializer.save() cc_thread = serializer.instance - thread_created.send(sender=None, user=user, post=cc_thread, notify_all_learners=notify_all_learners) + # Use send_signal_after_commit() to ensure the signal is sent only after the transaction commits. + send_signal_after_commit( + lambda: thread_created.send(sender=None, user=user, post=cc_thread, notify_all_learners=notify_all_learners) + ) api_thread = serializer.data _do_extra_actions(api_thread, cc_thread, list(thread_data.keys()), actions_form, context, request) @@ -1550,7 +1564,9 @@ def create_comment(request, comment_data): context["cc_requester"].follow(cc_thread) serializer.save() cc_comment = serializer.instance - comment_created.send(sender=None, user=request.user, post=cc_comment) + send_signal_after_commit( + lambda: comment_created.send(sender=None, user=request.user, post=cc_comment) + ) api_comment = serializer.data _do_extra_actions(api_comment, cc_comment, list(comment_data.keys()), actions_form, context, request) track_comment_created_event(request, course, cc_comment, cc_thread["commentable_id"], followed=False, @@ -1586,7 +1602,9 @@ def update_thread(request, thread_id, update_data): if set(update_data) - set(actions_form.fields): serializer.save() # signal to update Teams when a user edits a thread - thread_edited.send(sender=None, user=request.user, post=cc_thread) + send_signal_after_commit( + lambda: thread_edited.send(sender=None, user=request.user, post=cc_thread) + ) api_thread = serializer.data _do_extra_actions(api_thread, cc_thread, list(update_data.keys()), actions_form, context, request) @@ -1635,7 +1653,9 @@ def update_comment(request, comment_id, update_data): # Only save comment object if some of the edited fields are in the comment data, not extra actions if set(update_data) - set(actions_form.fields): serializer.save() - comment_edited.send(sender=None, user=request.user, post=cc_comment) + send_signal_after_commit( + lambda: comment_edited.send(sender=None, user=request.user, post=cc_comment) + ) api_comment = serializer.data _do_extra_actions(api_comment, cc_comment, list(update_data.keys()), actions_form, context, request) _handle_comment_signals(update_data, cc_comment, request.user) @@ -1823,7 +1843,9 @@ def delete_thread(request, thread_id): cc_thread, context = _get_thread_and_context(request, thread_id) if can_delete(cc_thread, context): cc_thread.delete() - thread_deleted.send(sender=None, user=request.user, post=cc_thread) + send_signal_after_commit( + lambda: thread_deleted.send(sender=None, user=request.user, post=cc_thread) + ) track_thread_deleted_event(request, context["course"], cc_thread) else: raise PermissionDenied @@ -1848,7 +1870,9 @@ def delete_comment(request, comment_id): cc_comment, context = _get_comment_and_context(request, comment_id) if can_delete(cc_comment, context): cc_comment.delete() - comment_deleted.send(sender=None, user=request.user, post=cc_comment) + send_signal_after_commit( + lambda: comment_deleted.send(sender=None, user=request.user, post=cc_comment) + ) track_comment_deleted_event(request, context["course"], cc_comment) else: raise PermissionDenied diff --git a/lms/djangoapps/discussion/rest_api/tests/test_api_v2.py b/lms/djangoapps/discussion/rest_api/tests/test_api_v2.py index 900d52017c5e..df4ac947bf0d 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_api_v2.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_api_v2.py @@ -273,7 +273,8 @@ def test_basic(self, mock_emit): with self.assert_signal_sent( api, "thread_created", sender=None, user=self.user, exclude_args=("post", "notify_all_learners") ): - actual = create_thread(self.request, self.minimal_data) + with self.captureOnCommitCallbacks(execute=True): + actual = create_thread(self.request, self.minimal_data) expected = self.expected_thread_data( { "id": "test_id", @@ -352,7 +353,8 @@ def test_basic_in_blackout_period_with_user_access(self, mock_emit): with self.assert_signal_sent( api, "thread_created", sender=None, user=self.user, exclude_args=("post", "notify_all_learners") ): - actual = create_thread(self.request, self.minimal_data) + with self.captureOnCommitCallbacks(execute=True): + actual = create_thread(self.request, self.minimal_data) expected = self.expected_thread_data( { "author_label": "Moderator", @@ -428,7 +430,8 @@ def test_title_truncation(self, mock_emit): with self.assert_signal_sent( api, "thread_created", sender=None, user=self.user, exclude_args=("post", "notify_all_learners") ): - create_thread(self.request, data) + with self.captureOnCommitCallbacks(execute=True): + create_thread(self.request, data) event_name, event_data = mock_emit.call_args[0] assert event_name == "edx.forum.thread.created" assert event_data == { @@ -678,7 +681,8 @@ def test_success(self, parent_id, mock_emit): with self.assert_signal_sent( api, "comment_created", sender=None, user=self.user, exclude_args=("post",) ): - actual = create_comment(self.request, data) + with self.captureOnCommitCallbacks(execute=True): + actual = create_comment(self.request, data) expected = { "id": "test_comment", "thread_id": "test_thread", @@ -785,7 +789,8 @@ def test_success_in_black_out_with_user_access(self, parent_id, mock_emit): with self.assert_signal_sent( api, "comment_created", sender=None, user=self.user, exclude_args=("post",) ): - actual = create_comment(self.request, data) + with self.captureOnCommitCallbacks(execute=True): + actual = create_comment(self.request, data) expected = { "id": "test_comment", "thread_id": "test_thread", @@ -1118,9 +1123,10 @@ def test_basic(self): with self.assert_signal_sent( api, "thread_edited", sender=None, user=self.user, exclude_args=("post",) ): - actual = update_thread( - self.request, "test_thread", {"raw_body": "Edited body"} - ) + with self.captureOnCommitCallbacks(execute=True): + actual = update_thread( + self.request, "test_thread", {"raw_body": "Edited body"} + ) assert actual == self.expected_thread_data( { @@ -1436,13 +1442,13 @@ def test_following(self, old_following, new_following, mock_emit): self.register_thread() data = {"following": new_following} signal_name = "thread_followed" if new_following else "thread_unfollowed" - mock_path = ( - f"openedx.core.djangoapps.django_comment_common.signals.{signal_name}.send" - ) + # Patch at the api module level where the signal is imported and used + mock_path = f"lms.djangoapps.discussion.rest_api.api.{signal_name}" with mock.patch(mock_path) as signal_patch: - result = update_thread(self.request, "test_thread", data) + with self.captureOnCommitCallbacks(execute=True): + result = update_thread(self.request, "test_thread", data) if old_following != new_following: - self.assertEqual(signal_patch.call_count, 1) + self.assertEqual(signal_patch.send.call_count, 1) assert result["following"] == new_following if old_following == new_following: @@ -1782,9 +1788,10 @@ def test_basic(self, parent_id): with self.assert_signal_sent( api, "comment_edited", sender=None, user=self.user, exclude_args=("post",) ): - actual = update_comment( - self.request, "test_comment", {"raw_body": "Edited body"} - ) + with self.captureOnCommitCallbacks(execute=True): + actual = update_comment( + self.request, "test_comment", {"raw_body": "Edited body"} + ) expected = { "anonymous": False, "anonymous_to_peers": False, @@ -2207,7 +2214,7 @@ def test_raw_body_access(self, role_name, is_thread_author, is_comment_author): ) @ddt.unpack @mock.patch( - "openedx.core.djangoapps.django_comment_common.signals.comment_endorsed.send" + "lms.djangoapps.discussion.rest_api.api.comment_endorsed.send" ) def test_endorsed_access( self, role_name, is_thread_author, thread_type, is_comment_author, endorsed_mock @@ -2226,7 +2233,8 @@ def test_endorsed_access( thread_type == "discussion" or not is_thread_author ) try: - update_comment(self.request, "test_comment", {"endorsed": True}) + with self.captureOnCommitCallbacks(execute=True): + update_comment(self.request, "test_comment", {"endorsed": True}) self.assertEqual(endorsed_mock.call_count, 1) assert not expected_error except ValidationError as err: @@ -2354,7 +2362,8 @@ def test_basic(self, mock_emit): with self.assert_signal_sent( api, "thread_deleted", sender=None, user=self.user, exclude_args=("post",) ): - assert delete_thread(self.request, self.thread_id) is None + with self.captureOnCommitCallbacks(execute=True): + assert delete_thread(self.request, self.thread_id) is None self.check_mock_called("delete_thread") params = { "thread_id": self.thread_id, @@ -2540,7 +2549,8 @@ def test_basic(self, mock_emit): with self.assert_signal_sent( api, "comment_deleted", sender=None, user=self.user, exclude_args=("post",) ): - assert delete_comment(self.request, self.comment_id) is None + with self.captureOnCommitCallbacks(execute=True): + assert delete_comment(self.request, self.comment_id) is None self.check_mock_called("delete_comment") params = { "comment_id": self.comment_id, diff --git a/lms/djangoapps/discussion/rest_api/utils.py b/lms/djangoapps/discussion/rest_api/utils.py index 0f02a0dcdcf2..a2591655adc2 100644 --- a/lms/djangoapps/discussion/rest_api/utils.py +++ b/lms/djangoapps/discussion/rest_api/utils.py @@ -3,13 +3,14 @@ """ import logging from datetime import datetime -from typing import Dict, List +from typing import Callable, Dict, List import requests from crum import get_current_request from django.conf import settings from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.core.paginator import Paginator +from django.db import transaction from django.db.models.functions import Length from pytz import UTC @@ -496,3 +497,24 @@ def get_captcha_site_key_by_platform(platform: str) -> str | None: Get reCAPTCHA site key based on the platform. """ return settings.RECAPTCHA_SITE_KEYS.get(platform, None) + + +def send_signal_after_commit(signal_func: Callable): + """ + Schedule a signal to be sent after the current database transaction commits. + + This helper ensures that signals are only sent after the transaction commits, + preventing race conditions where async tasks (like Celery workers) may try to + access database records before they are visible (especially important for MySQL + backend with transaction isolation). + + Args: + signal_func: A callable that sends the signal. This will be executed + after the transaction commits. + + Example: + send_signal_after_commit( + lambda: thread_created.send(sender=None, user=user, post=thread, notify_all_learners=False) + ) + """ + transaction.on_commit(signal_func) From f1d41653fae9011dea1b5e63df2c37ab3fe15034 Mon Sep 17 00:00:00 2001 From: Daniel Wong Date: Wed, 19 Nov 2025 21:42:27 -0600 Subject: [PATCH 11/21] feat: include user and origin_server info in library archive (#37626) --- openedx/core/djangoapps/content_libraries/tasks.py | 5 ++++- .../core/djangoapps/content_libraries/tests/test_tasks.py | 7 +++++++ requirements/constraints.txt | 2 +- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 7 files changed, 16 insertions(+), 6 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index d4e6196e03a2..c54779a8e621 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -27,6 +27,7 @@ from django.core.files.base import ContentFile from django.contrib.auth import get_user_model from django.core.serializers.json import DjangoJSONEncoder +from django.conf import settings from celery import shared_task from celery.utils.log import get_task_logger from celery_utils.logged_task import LoggedTask @@ -557,7 +558,9 @@ def backup_library(self, user_id: int, library_key_str: str) -> None: timestamp = datetime.now().strftime("%Y-%m-%d-%H%M%S") filename = f'{sanitized_lib_key}-{timestamp}.zip' file_path = os.path.join(root_dir, filename) - create_lib_zip_file(lp_key=str(library_key), path=file_path) + user = User.objects.get(id=user_id) + origin_server = getattr(settings, 'CMS_BASE', None) + create_lib_zip_file(lp_key=str(library_key), path=file_path, user=user, origin_server=origin_server) set_custom_attribute("exporting_completed", str(library_key)) with open(file_path, 'rb') as zipfile: diff --git a/openedx/core/djangoapps/content_libraries/tests/test_tasks.py b/openedx/core/djangoapps/content_libraries/tests/test_tasks.py index 4098b2a8fff9..ac64bc86ef28 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_tasks.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_tasks.py @@ -2,6 +2,7 @@ Unit tests for content libraries Celery tasks """ +from django.test import override_settings from ..models import ContentLibrary from .base import ContentLibrariesRestApiTest @@ -28,6 +29,7 @@ def test_backup_task_returns_task_id(self): result = backup_library.delay(self.user.id, str(self.lib1.library_key)) assert result.task_id is not None + @override_settings(CMS_BASE="test.com") def test_backup_task_success(self): result = backup_library.delay(self.user.id, str(self.lib1.library_key)) assert result.state == 'SUCCESS' @@ -35,6 +37,11 @@ def test_backup_task_success(self): artifact = UserTaskArtifact.objects.filter(status__task_id=result.task_id, name='Output').first() assert artifact is not None assert artifact.file.name.endswith('.zip') + # test artifact content + with artifact.file.open('rb') as f: + content = f.read() + assert b'created_by_email = "bob@example.com"' in content + assert b'origin_server = "test.com"' in content def test_backup_task_failure(self): result = backup_library.delay(self.user.id, self.wrong_task_id) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index c96838f93777..6b5f3e38c2ed 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -61,7 +61,7 @@ numpy<2.0.0 # Date: 2023-09-18 # pinning this version to avoid updates while the library is being developed # Issue for unpinning: https://github.com/openedx/edx-platform/issues/35269 -openedx-learning==0.30.0 +openedx-learning==0.30.1 # Date: 2023-11-29 # Open AI version 1.0.0 dropped support for openai.ChatCompletion which is currently in use in enterprise. diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index f3387eb7bb18..654886be2a01 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -855,7 +855,7 @@ openedx-filters==2.1.0 # ora2 openedx-forum==0.3.8 # via -r requirements/edx/kernel.in -openedx-learning==0.30.0 +openedx-learning==0.30.1 # via # -c requirements/constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 6e75615ec812..1278b4cfe9e2 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1419,7 +1419,7 @@ openedx-forum==0.3.8 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt -openedx-learning==0.30.0 +openedx-learning==0.30.1 # via # -c requirements/constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 5e465e8905ba..3adffb5dd3eb 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -1033,7 +1033,7 @@ openedx-filters==2.1.0 # ora2 openedx-forum==0.3.8 # via -r requirements/edx/base.txt -openedx-learning==0.30.0 +openedx-learning==0.30.1 # via # -c requirements/constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 08f7639ace60..5c4e0a9b48c4 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1079,7 +1079,7 @@ openedx-filters==2.1.0 # ora2 openedx-forum==0.3.8 # via -r requirements/edx/base.txt -openedx-learning==0.30.0 +openedx-learning==0.30.1 # via # -c requirements/constraints.txt # -r requirements/edx/base.txt From 98a9ee286df6ab9694f6060947eb8f9cee4197b7 Mon Sep 17 00:00:00 2001 From: Felipe Montoya Date: Wed, 26 Nov 2025 15:38:32 -0500 Subject: [PATCH 12/21] chore: change release line from 'master' to 'ulmo' --- openedx/core/release.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/release.py b/openedx/core/release.py index ce30df8cc543..dda3add782a0 100644 --- a/openedx/core/release.py +++ b/openedx/core/release.py @@ -8,7 +8,7 @@ # The release line: an Open edX release name ("ficus"), or "master". # This should always be "master" on the master branch, and will be changed # manually when we start release-line branches, like open-release/ficus.master. -RELEASE_LINE = "master" +RELEASE_LINE = "ulmo" def doc_version(): From cf48323639bf24eed5ef120dfbd9e98cf0fd64af Mon Sep 17 00:00:00 2001 From: mariajgrimaldi <64440265+mariajgrimaldi@users.noreply.github.com> Date: Thu, 27 Nov 2025 17:16:07 +0000 Subject: [PATCH 13/21] feat: Upgrade Python dependency openedx-authz Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master` --- requirements/edx/base.txt | 3 ++- requirements/edx/development.txt | 3 ++- requirements/edx/doc.txt | 3 ++- requirements/edx/testing.txt | 3 ++- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 654886be2a01..60e19cd22853 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -465,6 +465,7 @@ edx-django-utils==8.0.1 # edx-when # enterprise-integrated-channels # event-tracking + # openedx-authz # openedx-events # ora2 # super-csv @@ -826,7 +827,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==0.19.0 +openedx-authz==0.20.0 # via -r requirements/edx/kernel.in openedx-calc==4.0.2 # via -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 1278b4cfe9e2..85ac66aed195 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -747,6 +747,7 @@ edx-django-utils==8.0.1 # edx-when # enterprise-integrated-channels # event-tracking + # openedx-authz # openedx-events # ora2 # super-csv @@ -1376,7 +1377,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==0.19.0 +openedx-authz==0.20.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 3adffb5dd3eb..68c115676fe9 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -553,6 +553,7 @@ edx-django-utils==8.0.1 # edx-when # enterprise-integrated-channels # event-tracking + # openedx-authz # openedx-events # ora2 # super-csv @@ -1003,7 +1004,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==0.19.0 +openedx-authz==0.20.0 # via -r requirements/edx/base.txt openedx-calc==4.0.2 # via -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 5c4e0a9b48c4..f58ef16f9e31 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -575,6 +575,7 @@ edx-django-utils==8.0.1 # edx-when # enterprise-integrated-channels # event-tracking + # openedx-authz # openedx-events # ora2 # super-csv @@ -1049,7 +1050,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==0.19.0 +openedx-authz==0.20.0 # via -r requirements/edx/base.txt openedx-calc==4.0.2 # via -r requirements/edx/base.txt From 0ee2faa6ea1208e0988ffe482ab1c7d456eccb61 Mon Sep 17 00:00:00 2001 From: Ahtisham Shahid Date: Tue, 9 Dec 2025 10:20:37 +0500 Subject: [PATCH 14/21] chore: updated pref settings for misc notification types (#37738) --- openedx/core/djangoapps/notifications/base_notification.py | 4 ++-- openedx/core/djangoapps/notifications/tests/test_views.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/notifications/base_notification.py b/openedx/core/djangoapps/notifications/base_notification.py index 5264053ace5a..4b9393830eb9 100644 --- a/openedx/core/djangoapps/notifications/base_notification.py +++ b/openedx/core/djangoapps/notifications/base_notification.py @@ -178,7 +178,7 @@ 'is_core': False, 'info': '', 'web': True, - 'email': False, + 'email': True, 'push': False, 'email_cadence': EmailCadence.DAILY, 'non_editable': ['push'], @@ -236,7 +236,7 @@ 'is_core': False, 'info': '', 'web': True, - 'email': False, + 'email': True, 'email_cadence': EmailCadence.DAILY, 'push': False, 'non_editable': ['push'], diff --git a/openedx/core/djangoapps/notifications/tests/test_views.py b/openedx/core/djangoapps/notifications/tests/test_views.py index 7148295dcf50..5e09a86c5914 100644 --- a/openedx/core/djangoapps/notifications/tests/test_views.py +++ b/openedx/core/djangoapps/notifications/tests/test_views.py @@ -604,7 +604,7 @@ def setUp(self): }, "new_instructor_all_learners_post": { "web": True, - "email": False, + "email": True, "push": False, "email_cadence": "Daily" }, @@ -628,7 +628,7 @@ def setUp(self): "notification_types": { "course_updates": { "web": True, - "email": False, + "email": True, "push": False, "email_cadence": "Daily" }, From 9956dd70e79cde6c63615369f858b6caf4c136c7 Mon Sep 17 00:00:00 2001 From: Asespinel <79876430+Asespinel@users.noreply.github.com> Date: Wed, 3 Dec 2025 11:28:49 -0500 Subject: [PATCH 15/21] fix: Course search pill not cleared when text deleted. (#37709) * fix: Course search pill not cleared when text deleted * chore: fix spacing Co-authored-by: Feanil Patel --------- Co-authored-by: Feanil Patel --- lms/static/js/discovery/discovery_factory.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lms/static/js/discovery/discovery_factory.js b/lms/static/js/discovery/discovery_factory.js index d26841f86fca..0e44d8154a84 100644 --- a/lms/static/js/discovery/discovery_factory.js +++ b/lms/static/js/discovery/discovery_factory.js @@ -30,8 +30,11 @@ } listing = new CoursesListing({model: courseListingModel}); - dispatcher.listenTo(form, 'search', function(query) { + dispatcher.listenTo(form, "search", function (query) { form.showLoadingIndicator(); + if (!query || query.trim() === "") { + filters.remove("search_query"); + } search.performSearch(query, filters.getTerms()); }); From f3b971913dd635fa46b86e52f10f52cec9030035 Mon Sep 17 00:00:00 2001 From: Asad Ali Date: Thu, 20 Nov 2025 22:05:14 +0500 Subject: [PATCH 16/21] revert: feat: [FC-0092] Optimize Course Info Blocks API (#37122) (#37661) This reverts commit 7cd4170ca7bfd29612fe4b8b469df539d64e327f. --- lms/djangoapps/course_api/blocks/api.py | 24 +------ lms/djangoapps/course_api/blocks/utils.py | 20 ------ lms/djangoapps/course_api/blocks/views.py | 43 ----------- lms/djangoapps/courseware/courses.py | 6 +- lms/djangoapps/grades/course_data.py | 8 +-- .../tests/test_course_info_views.py | 72 ------------------- 6 files changed, 5 insertions(+), 168 deletions(-) diff --git a/lms/djangoapps/course_api/blocks/api.py b/lms/djangoapps/course_api/blocks/api.py index 80c2bb7fc5ff..a79e9759e191 100644 --- a/lms/djangoapps/course_api/blocks/api.py +++ b/lms/djangoapps/course_api/blocks/api.py @@ -1,7 +1,7 @@ """ API function for retrieving course blocks data """ -from edx_django_utils.cache import RequestCache + import lms.djangoapps.course_blocks.api as course_blocks_api from lms.djangoapps.course_blocks.transformers.access_denied_filter import AccessDeniedMessageFilterTransformer @@ -14,7 +14,6 @@ from .toggles import HIDE_ACCESS_DENIALS_FLAG from .transformers.blocks_api import BlocksAPITransformer from .transformers.milestones import MilestonesAndSpecialExamsTransformer -from .utils import COURSE_API_REQUEST_CACHE_NAMESPACE, REUSABLE_BLOCKS_CACHE_KEY def get_blocks( @@ -30,7 +29,6 @@ def get_blocks( block_types_filter=None, hide_access_denials=False, allow_start_dates_in_future=False, - cache_with_future_dates=False, ): """ Return a serialized representation of the course blocks. @@ -63,7 +61,6 @@ def get_blocks( allow_start_dates_in_future (bool): When True, will allow blocks to be returned that can bypass the StartDateTransformer's filter to show blocks with start dates in the future. - cache_with_future_dates (bool): When True, will use the block caching logic using RequestCache """ if HIDE_ACCESS_DENIALS_FLAG.is_enabled(): @@ -121,10 +118,6 @@ def get_blocks( ), ] - if cache_with_future_dates: - # Include future dates such that get_course_assignments can reuse the block structure from RequestCache - allow_start_dates_in_future = True - # transform blocks = course_blocks_api.get_course_blocks( user, @@ -135,19 +128,6 @@ def get_blocks( include_has_scheduled_content=include_has_scheduled_content ) - if cache_with_future_dates: - # Store a copy of the transformed, but still unfiltered, course blocks in RequestCache to be reused - # wherever possible for optimization. Copying is required to make sure the cached structure is not mutated - # by the filtering below. - request_cache = RequestCache(COURSE_API_REQUEST_CACHE_NAMESPACE) - request_cache.set(REUSABLE_BLOCKS_CACHE_KEY, blocks.copy()) - - # Since we included blocks with future start dates in our block structure, - # we need to include the 'start' field to filter out such blocks before returning the response. - # If 'start' field is not requested, it will be removed from the response. - requested_fields = set(requested_fields) - requested_fields.add('start') - # filter blocks by types if block_types_filter: block_keys_to_remove = [] @@ -162,7 +142,7 @@ def get_blocks( serializer_context = { 'request': request, 'block_structure': blocks, - 'requested_fields': requested_fields, + 'requested_fields': requested_fields or [], } if return_type == 'dict': diff --git a/lms/djangoapps/course_api/blocks/utils.py b/lms/djangoapps/course_api/blocks/utils.py index 0686abc2fac1..6f371624b7df 100644 --- a/lms/djangoapps/course_api/blocks/utils.py +++ b/lms/djangoapps/course_api/blocks/utils.py @@ -1,7 +1,6 @@ """ Utils for Blocks """ -from edx_django_utils.cache import RequestCache from rest_framework.utils.serializer_helpers import ReturnList from openedx.core.djangoapps.discussions.models import ( @@ -10,10 +9,6 @@ ) -COURSE_API_REQUEST_CACHE_NAMESPACE = "course_api" -REUSABLE_BLOCKS_CACHE_KEY = "reusable_transformed_blocks" - - def filter_discussion_xblocks_from_response(response, course_key): """ Removes discussion xblocks if discussion provider is openedx. @@ -68,18 +63,3 @@ def filter_discussion_xblocks_from_response(response, course_key): response.data['blocks'] = filtered_blocks return response - - -def get_cached_transformed_blocks(): - """ - Helper function to get an unfiltered course structure from RequestCache, - including blocks with start dates in the future. - - Caution: For performance reasons, the function returns the structure object itself, not its copy. - This means the retrieved structure is supposed to be read-only and should not be mutated by consumers. - """ - request_cache = RequestCache(COURSE_API_REQUEST_CACHE_NAMESPACE) - cached_response = request_cache.get_cached_response(REUSABLE_BLOCKS_CACHE_KEY) - reusable_transformed_blocks = cached_response.value if cached_response.is_found else None - - return reusable_transformed_blocks diff --git a/lms/djangoapps/course_api/blocks/views.py b/lms/djangoapps/course_api/blocks/views.py index 7f81861b9b7b..96679a562957 100644 --- a/lms/djangoapps/course_api/blocks/views.py +++ b/lms/djangoapps/course_api/blocks/views.py @@ -2,7 +2,6 @@ CourseBlocks API views """ -from datetime import datetime, timezone from django.core.exceptions import ValidationError from django.db import transaction @@ -238,7 +237,6 @@ def list(self, request, usage_key_string, hide_access_denials=False): # pylint: params.cleaned_data['return_type'], params.cleaned_data.get('block_types_filter', None), hide_access_denials=hide_access_denials, - cache_with_future_dates=True ) ) # If the username is an empty string, and not None, then we are requesting @@ -341,50 +339,9 @@ def list(self, request, hide_access_denials=False): # pylint: disable=arguments if not root: raise ValidationError(f"Unable to find course block in '{course_key_string}'") - # Earlier we included blocks with future start dates in the collected/cached block structure. - # Now we need to emulate allow_start_dates_in_future=False by removing any such blocks. - include_start = "start" in request.query_params['requested_fields'] - self.remove_future_blocks(course_blocks, include_start) - recurse_mark_complete(root, course_blocks) return response - @staticmethod - def remove_future_blocks(course_blocks, include_start: bool): - """ - Mutates course_blocks in place: - - removes blocks whose 'start' is in the future - - also removes references to them from parents' 'children' lists - - removes 'start' key from all blocks if it wasn't requested - """ - if not course_blocks: - return course_blocks - - now = datetime.now(timezone.utc) - - # 1. Collect IDs of blocks to remove - to_remove = set() - for block_id, block in course_blocks.items(): - get_field = block.get if include_start else block.pop - start = get_field("start") - if start and start > now: - to_remove.add(block_id) - - if not to_remove: - return course_blocks - - # 2. Remove the blocks themselves - for block_id in to_remove: - course_blocks.pop(block_id, None) - - # 3. Clean up children lists - for block in course_blocks.values(): - children = block.get("children") - if children: - block["children"] = [cid for cid in children if cid not in to_remove] - - return course_blocks - @method_decorator(transaction.non_atomic_requests, name='dispatch') @view_auth_classes(is_authenticated=False) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 3b527cda4dfa..bbf9d5394705 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -26,7 +26,6 @@ from common.djangoapps.static_replace import replace_static_urls from common.djangoapps.util.date_utils import strftime_localized from lms.djangoapps import branding -from lms.djangoapps.course_api.blocks.utils import get_cached_transformed_blocks from lms.djangoapps.course_blocks.api import get_course_blocks from lms.djangoapps.courseware.access import has_access from lms.djangoapps.courseware.access_response import ( @@ -637,10 +636,7 @@ def get_course_assignments(course_key, user, include_access=False, include_witho store = modulestore() course_usage_key = store.make_course_usage_key(course_key) - - block_data = get_cached_transformed_blocks() or get_course_blocks( - user, course_usage_key, allow_start_dates_in_future=True, include_completion=True - ) + block_data = get_course_blocks(user, course_usage_key, allow_start_dates_in_future=True, include_completion=True) now = datetime.now(pytz.UTC) assignments = [] diff --git a/lms/djangoapps/grades/course_data.py b/lms/djangoapps/grades/course_data.py index 523d6e6df38d..5464c4f88105 100644 --- a/lms/djangoapps/grades/course_data.py +++ b/lms/djangoapps/grades/course_data.py @@ -2,12 +2,12 @@ Code used to get and cache the requested course-data """ + from lms.djangoapps.course_blocks.api import get_course_blocks from openedx.core.djangoapps.content.block_structure.api import get_block_structure_manager from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from .transformer import GradesTransformer -from ..course_api.blocks.utils import get_cached_transformed_blocks class CourseData: @@ -56,11 +56,7 @@ def location(self): # lint-amnesty, pylint: disable=missing-function-docstring @property def structure(self): # lint-amnesty, pylint: disable=missing-function-docstring if self._structure is None: - # The get_course_blocks function proved to be a major time sink during a request at "blocks/". - # This caching logic helps improve the response time by getting a copy of the already transformed, but still - # unfiltered, course blocks from RequestCache and thus reducing the number of times that - # the get_course_blocks function is called. - self._structure = get_cached_transformed_blocks() or get_course_blocks( + self._structure = get_course_blocks( self.user, self.location, collected_block_structure=self._collected_block_structure, diff --git a/lms/djangoapps/mobile_api/tests/test_course_info_views.py b/lms/djangoapps/mobile_api/tests/test_course_info_views.py index b7cdc6b03961..efb3f7d9fdbb 100644 --- a/lms/djangoapps/mobile_api/tests/test_course_info_views.py +++ b/lms/djangoapps/mobile_api/tests/test_course_info_views.py @@ -432,78 +432,6 @@ def test_extend_sequential_info_with_assignment_progress_for_other_types(self, b for block_info in response.data['blocks'].values(): self.assertNotEqual('assignment_progress', block_info) - def test_response_keys(self): - response = self.verify_response(url=self.url) - data = response.data - - expected_top_level_keys = { - 'blocks', - 'certificate', - 'course_about', - 'course_access_details', - 'course_handouts', - 'course_modes', - 'course_progress', - 'course_sharing_utm_parameters', - 'course_updates', - 'deprecate_youtube', - 'discussion_url', - 'end', - 'enrollment_details', - 'id', - 'is_self_paced', - 'media', - 'name', - 'number', - 'org', - 'org_logo', - 'root', - 'start', - 'start_display', - 'start_type' - } - expected_course_access_keys = { - "has_unmet_prerequisites", - "is_too_early", - "is_staff", - "audit_access_expires", - "courseware_access" - } - expected_courseware_access_keys = { - "has_access", - "error_code", - "developer_message", - "user_message", - "additional_context_user_message", - "user_fragment" - } - expected_enrollment_details_keys = {"created", "mode", "is_active", "upgrade_deadline"} - expected_media_keys = {"image"} - expected_image_keys = {"raw", "small", "large"} - expected_course_sharing_keys = {"facebook", "twitter"} - expected_course_modes_keys = {"slug", "sku", "android_sku", "ios_sku", "min_price"} - expected_course_progress_keys = {"total_assignments_count", "assignments_completed"} - - self.assertSetEqual(set(data), expected_top_level_keys) - self.assertSetEqual(set(data["course_access_details"]), expected_course_access_keys) - self.assertSetEqual(set(data["course_access_details"]["courseware_access"]), expected_courseware_access_keys) - self.assertSetEqual(set(data["enrollment_details"]), expected_enrollment_details_keys) - self.assertSetEqual(set(data["media"]), expected_media_keys) - self.assertSetEqual(set(data["media"]["image"]), expected_image_keys) - self.assertSetEqual(set(data["course_sharing_utm_parameters"]), expected_course_sharing_keys) - self.assertSetEqual(set(data["course_modes"][0]), expected_course_modes_keys) - self.assertSetEqual(set(data["course_progress"]), expected_course_progress_keys) - - def test_block_count_depends_on_depth_in_request_params(self): - response_depth_zero = self.verify_response(url=self.url, params={'depth': 0}) - response_depth_one = self.verify_response(url=self.url, params={'depth': 1}) - blocks_depth_zero = [block for block in self.store.get_items(self.course_key) if block.category == "course"] - blocks_depth_one = [ - block for block in self.store.get_items(self.course_key) if block.category in ("course", "chapter") - ] - self.assertEqual(len(response_depth_zero.data["blocks"]), len(blocks_depth_zero)) - self.assertEqual(len(response_depth_one.data["blocks"]), len(blocks_depth_one)) - class TestCourseEnrollmentDetailsView(MobileAPITestCase, MilestonesTestCaseMixin): # lint-amnesty, pylint: disable=test-inherits-tests """ From ba5113c446027a03db25ea82c504d72293f72962 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 3 Dec 2025 16:49:08 -0500 Subject: [PATCH 17/21] fix: don't send emails on library backup/restore There is no way to resume either the backup or restore library actions, i.e. if you navigate away from it, you have to do it again. This is a limitation of the current UI because we wanted to get something quick and simple in for Ulmo, but it also reflects the fact that library backup/restore should be much faster than course import/export has historically been. In any case, sending an email for a 5-10 second task is unnecessary and distracting, so this commit suppresses the email. Note: I'm using local imports to get around the fact that the content_libraries public API is used by content_libraries/tasks.py which defines the tasks. I can't import from content_libraries/tasks.py directly, because that would violate import linter rules forbidding other apps from importing things outside of api.py. This isn't ideal, but it keeps the fix small and it keeps the logic in the content_libraries app. --- cms/djangoapps/cms_user_tasks/signals.py | 25 ++++++++++++++++++- .../content_libraries/api/libraries.py | 14 +++++++++++ .../djangoapps/content_libraries/tasks.py | 7 ++++-- .../content_libraries/tests/test_tasks.py | 10 ++++++-- 4 files changed, 51 insertions(+), 5 deletions(-) diff --git a/cms/djangoapps/cms_user_tasks/signals.py b/cms/djangoapps/cms_user_tasks/signals.py index 40bfd5781825..e6ddba747d5f 100644 --- a/cms/djangoapps/cms_user_tasks/signals.py +++ b/cms/djangoapps/cms_user_tasks/signals.py @@ -12,6 +12,7 @@ from cms.djangoapps.contentstore.toggles import bypass_olx_failure_enabled from cms.djangoapps.contentstore.utils import course_import_olx_validation_is_enabled +from openedx.core.djangoapps.content_libraries.api import is_library_backup_task, is_library_restore_task from .tasks import send_task_complete_email @@ -64,6 +65,28 @@ def get_olx_validation_from_artifact(): if olx_artifact and not bypass_olx_failure_enabled(): return olx_artifact.text + def should_skip_end_of_task_email(task_name) -> bool: + """ + Studio tasks generally send an email when finished, but not always. + + Some tasks can last many minutes, e.g. course import/export. For these + tasks, there is a high chance that the user has navigated away and will + want to check back in later. Yet email notification is unnecessary and + distracting for things like the Library restore task, which is + relatively quick and cannot be resumed (i.e. if you navigate away, you + have to upload again). + + The task_name passed in will be lowercase. + """ + # We currently have to pattern match on the name to differentiate + # between tasks. A better long term solution would be to add a separate + # task type identifier field to Django User Tasks. + return ( + is_library_content_update(task_name) or + is_library_backup_task(task_name) or + is_library_restore_task(task_name) + ) + status = kwargs['status'] # Only send email when the entire task is complete, should only send when @@ -72,7 +95,7 @@ def get_olx_validation_from_artifact(): task_name = status.name.lower() # Also suppress emails on library content XBlock updates (too much like spam) - if is_library_content_update(task_name): + if should_skip_end_of_task_email(task_name): LOGGER.info(f"Suppressing end-of-task email on task {task_name}") return diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index 66281addb643..d77061d583be 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -108,6 +108,8 @@ "get_backup_task_status", "assign_library_role_to_user", "user_has_permission_across_lib_authz_systems", + "is_library_backup_task", + "is_library_restore_task", ] @@ -851,3 +853,15 @@ def _is_legacy_permission(permission: str) -> bool: or the new openedx-authz system. """ return permission in LEGACY_LIB_PERMISSIONS + + +def is_library_backup_task(task_name: str) -> bool: + """Case-insensitive match to see if a task is a library backup.""" + from ..tasks import LibraryBackupTask # avoid circular import error + return task_name.startswith(LibraryBackupTask.NAME_PREFIX.lower()) + + +def is_library_restore_task(task_name: str) -> bool: + """Case-insensitive match to see if a task is a library restore.""" + from ..tasks import LibraryRestoreTask # avoid circular import error + return task_name.startswith(LibraryRestoreTask.NAME_PREFIX.lower()) diff --git a/openedx/core/djangoapps/content_libraries/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index c54779a8e621..bbbf847bfbc7 100644 --- a/openedx/core/djangoapps/content_libraries/tasks.py +++ b/openedx/core/djangoapps/content_libraries/tasks.py @@ -512,6 +512,7 @@ class LibraryBackupTask(UserTask): # pylint: disable=abstract-method """ Base class for tasks related with Library backup functionality. """ + NAME_PREFIX = "Library Learning Package Backup" @classmethod def generate_name(cls, arguments_dict) -> str: @@ -529,7 +530,7 @@ def generate_name(cls, arguments_dict) -> str: str: The generated name """ key = arguments_dict['library_key_str'] - return f'Backup of {key}' + return f'{cls.NAME_PREFIX} of {key}' @shared_task(base=LibraryBackupTask, bind=True) @@ -591,10 +592,12 @@ class LibraryRestoreTask(UserTask): ERROR_LOG_ARTIFACT_NAME = 'Error log' + NAME_PREFIX = "Library Learning Package Restore" + @classmethod def generate_name(cls, arguments_dict): storage_path = arguments_dict['storage_path'] - return f'learning package restore of {storage_path}' + return f'{cls.NAME_PREFIX} of {storage_path}' def fail_with_error_log(self, logfile) -> None: """ diff --git a/openedx/core/djangoapps/content_libraries/tests/test_tasks.py b/openedx/core/djangoapps/content_libraries/tests/test_tasks.py index ac64bc86ef28..a200471c00bd 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_tasks.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_tasks.py @@ -1,6 +1,7 @@ """ Unit tests for content libraries Celery tasks """ +from unittest import mock from django.test import override_settings from ..models import ContentLibrary @@ -14,6 +15,7 @@ class ContentLibraryBackupTaskTest(ContentLibrariesRestApiTest): """ Tests for Content Library export task. """ + SEND_TASK_COMPLETE_FN = 'cms.djangoapps.cms_user_tasks.tasks.send_task_complete_email.delay' def setUp(self) -> None: super().setUp() @@ -31,7 +33,9 @@ def test_backup_task_returns_task_id(self): @override_settings(CMS_BASE="test.com") def test_backup_task_success(self): - result = backup_library.delay(self.user.id, str(self.lib1.library_key)) + with mock.patch(self.SEND_TASK_COMPLETE_FN) as send_task_complete_email: + result = backup_library.delay(self.user.id, str(self.lib1.library_key)) + send_task_complete_email.assert_not_called() assert result.state == 'SUCCESS' # Ensure an artifact was created with the output file artifact = UserTaskArtifact.objects.filter(status__task_id=result.task_id, name='Output').first() @@ -44,7 +48,9 @@ def test_backup_task_success(self): assert b'origin_server = "test.com"' in content def test_backup_task_failure(self): - result = backup_library.delay(self.user.id, self.wrong_task_id) + with mock.patch(self.SEND_TASK_COMPLETE_FN) as send_task_complete_email: + result = backup_library.delay(self.user.id, self.wrong_task_id) + send_task_complete_email.assert_not_called() assert result.state == 'FAILURE' # Ensure an error artifact was created artifact = UserTaskArtifact.objects.filter(status__task_id=result.task_id, name='Error').first() From 9091801260359a07a465ecc3a1a89a19c2fc032e Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 11 Dec 2025 11:18:06 -0500 Subject: [PATCH 18/21] fix: CourseLimitedStaffRole should not be able to access studio. We previously fixed this when the CourseLimitedStaffRole was applied to a course but did not handle the case where the role is applied to a user for a whole org. The underlying issue is that the CourseLimitedStaffRole is a subclass of the CourseStaffRole and much of the system assumes that subclesses are for giving more access not less access. To prevent that from happening for the case of the CourseLimitedStaffRole, when we do CourseStaffRole access checks, we use the strict_role_checking context manager to ensure that we're not accidentally granting the limited_staff role too much access. --- .../contentstore/tests/test_course_listing.py | 44 +++++++++++++++++++ cms/djangoapps/contentstore/views/course.py | 5 ++- common/djangoapps/student/auth.py | 6 ++- common/djangoapps/student/tests/test_authz.py | 18 ++++++++ 4 files changed, 70 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index d256228228cb..990eff83c922 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/cms/djangoapps/contentstore/tests/test_course_listing.py @@ -21,8 +21,10 @@ get_courses_accessible_to_user ) from common.djangoapps.course_action_state.models import CourseRerunState +from common.djangoapps.student.models.user import CourseAccessRole from common.djangoapps.student.roles import ( CourseInstructorRole, + CourseLimitedStaffRole, CourseStaffRole, GlobalStaff, OrgInstructorRole, @@ -176,6 +178,48 @@ def test_staff_course_listing(self): with self.assertNumQueries(2): list(_accessible_courses_summary_iter(self.request)) + def test_course_limited_staff_course_listing(self): + # Setup a new course + course_location = self.store.make_course_key('Org', 'CreatedCourse', 'Run') + CourseFactory.create( + org=course_location.org, + number=course_location.course, + run=course_location.run + ) + course = CourseOverviewFactory.create(id=course_location, org=course_location.org) + + # Add the user as a course_limited_staff on the course + CourseLimitedStaffRole(course.id).add_users(self.user) + self.assertTrue(CourseLimitedStaffRole(course.id).has_user(self.user)) + + # Fetch accessible courses list & verify their count + courses_list_by_staff, __ = get_courses_accessible_to_user(self.request) + + # Limited Course Staff should not be able to list courses in Studio + assert len(list(courses_list_by_staff)) == 0 + + def test_org_limited_staff_course_listing(self): + + # Setup a new course + course_location = self.store.make_course_key('Org', 'CreatedCourse', 'Run') + CourseFactory.create( + org=course_location.org, + number=course_location.course, + run=course_location.run + ) + course = CourseOverviewFactory.create(id=course_location, org=course_location.org) + + # Add a user as course_limited_staff on the org + # This is not possible using the course roles classes but is possible via Django admin so we + # insert a row into the model directly to test that scenario. + CourseAccessRole.objects.create(user=self.user, org=course_location.org, role=CourseLimitedStaffRole.ROLE) + + # Fetch accessible courses list & verify their count + courses_list_by_staff, __ = get_courses_accessible_to_user(self.request) + + # Limited Course Staff should not be able to list courses in Studio + assert len(list(courses_list_by_staff)) == 0 + def test_get_course_list_with_invalid_course_location(self): """ Test getting courses with invalid course location (course deleted from modulestore). diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 453e30e0aad0..f965ebb7aaa7 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -61,6 +61,7 @@ GlobalStaff, UserBasedRole, OrgStaffRole, + strict_role_checking, ) from common.djangoapps.util.json_request import JsonResponse, JsonResponseBadRequest, expect_json from common.djangoapps.util.string_utils import _has_non_ascii_characters @@ -536,7 +537,9 @@ def filter_ccx(course_access): return not isinstance(course_access.course_id, CCXLocator) instructor_courses = UserBasedRole(request.user, CourseInstructorRole.ROLE).courses_with_role() - staff_courses = UserBasedRole(request.user, CourseStaffRole.ROLE).courses_with_role() + with strict_role_checking(): + staff_courses = UserBasedRole(request.user, CourseStaffRole.ROLE).courses_with_role() + all_courses = list(filter(filter_ccx, instructor_courses | staff_courses)) courses_list = [] course_keys = {} diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index e199142fe377..047f0174a062 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -24,6 +24,7 @@ OrgInstructorRole, OrgLibraryUserRole, OrgStaffRole, + strict_role_checking, ) # Studio permissions: @@ -115,8 +116,9 @@ def get_user_permissions(user, course_key, org=None, service_variant=None): return STUDIO_NO_PERMISSIONS # Staff have all permissions except EDIT_ROLES: - if OrgStaffRole(org=org).has_user(user) or (course_key and user_has_role(user, CourseStaffRole(course_key))): - return STUDIO_VIEW_USERS | STUDIO_EDIT_CONTENT | STUDIO_VIEW_CONTENT + with strict_role_checking(): + if OrgStaffRole(org=org).has_user(user) or (course_key and user_has_role(user, CourseStaffRole(course_key))): + return STUDIO_VIEW_USERS | STUDIO_EDIT_CONTENT | STUDIO_VIEW_CONTENT # Otherwise, for libraries, users can view only: if course_key and isinstance(course_key, LibraryLocator): diff --git a/common/djangoapps/student/tests/test_authz.py b/common/djangoapps/student/tests/test_authz.py index c0b88e6318b5..70636e04b68a 100644 --- a/common/djangoapps/student/tests/test_authz.py +++ b/common/djangoapps/student/tests/test_authz.py @@ -11,6 +11,7 @@ from django.test import TestCase, override_settings from opaque_keys.edx.locator import CourseLocator +from common.djangoapps.student.models.user import CourseAccessRole from common.djangoapps.student.auth import ( add_users, has_studio_read_access, @@ -305,6 +306,23 @@ def test_limited_staff_no_studio_access_cms(self): assert not has_studio_read_access(self.limited_staff, self.course_key) assert not has_studio_write_access(self.limited_staff, self.course_key) + @override_settings(SERVICE_VARIANT='cms') + def test_limited_org_staff_no_studio_access_cms(self): + """ + Verifies that course limited staff have no read and no write access when SERVICE_VARIANT is not 'lms'. + """ + # Add a user as course_limited_staff on the org + # This is not possible using the course roles classes but is possible via Django admin so we + # insert a row into the model directly to test that scenario. + CourseAccessRole.objects.create( + user=self.limited_staff, + org=self.course_key.org, + role=CourseLimitedStaffRole.ROLE, + ) + + assert not has_studio_read_access(self.limited_staff, self.course_key) + assert not has_studio_write_access(self.limited_staff, self.course_key) + class CourseOrgGroupTest(TestCase): """ From dd91e54237faff324bbff4e1f00d5defb26406c6 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Fri, 12 Dec 2025 10:02:41 -0500 Subject: [PATCH 19/21] fix: sanitize HTML for course overview & sidebar The "overview" and "about_sidebar_html" fields in the CoursewareInformation view (/api/courseware/course/{courseId}) were returning unsanitized HTML and relying on the client to sanitize it. This commit shifts that work to the server side (clean_dangerous_html) to remove potentially dangerous tags when generating the response. The source of this data is modified in the "Settings and Details" section of a course in Studio. --- .../core/djangoapps/courseware_api/tests/test_views.py | 9 +++++++-- openedx/core/djangoapps/courseware_api/views.py | 9 +++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/courseware_api/tests/test_views.py b/openedx/core/djangoapps/courseware_api/tests/test_views.py index 1606d245c01f..62fd6d557b34 100644 --- a/openedx/core/djangoapps/courseware_api/tests/test_views.py +++ b/openedx/core/djangoapps/courseware_api/tests/test_views.py @@ -662,15 +662,20 @@ def test_invitation_only_property(self, invitation_only): ) def test_about_sidebar_html_property(self, waffle_enabled, mock_get_course_about_section): """ - Test about_sidebar_html property with different waffle settings + Test about_sidebar_html property with different waffle settings. + + Ensure that when a value is returned, ' with override_waffle_switch(ENABLE_COURSE_ABOUT_SIDEBAR_HTML, active=waffle_enabled): meta = self.create_courseware_meta() if waffle_enabled: assert meta.about_sidebar_html == '
About Course
' else: assert meta.about_sidebar_html is None + assert meta.overview == '
About Course
' @ddt.ddt diff --git a/openedx/core/djangoapps/courseware_api/views.py b/openedx/core/djangoapps/courseware_api/views.py index 1dcfc740c84c..a5940d8a132e 100644 --- a/openedx/core/djangoapps/courseware_api/views.py +++ b/openedx/core/djangoapps/courseware_api/views.py @@ -63,6 +63,7 @@ from openedx.core.djangoapps.courseware_api.utils import get_celebrations_dict from openedx.core.djangoapps.enrollments.permissions import ENROLL_IN_COURSE from openedx.core.djangoapps.programs.utils import ProgramProgressMeter +from openedx.core.djangolib.markup import clean_dangerous_html from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin from openedx.core.lib.courses import get_course_by_id @@ -516,7 +517,9 @@ def about_sidebar_html(self): Returns the HTML content for the course about section. """ if ENABLE_COURSE_ABOUT_SIDEBAR_HTML.is_enabled(): - return get_course_about_section(self.request, self.course, "about_sidebar_html") + return clean_dangerous_html( + get_course_about_section(self.request, self.course, "about_sidebar_html") + ) return None @property @@ -524,7 +527,9 @@ def overview(self): """ Returns the overview HTML content for the course. """ - return get_course_about_section(self.request, self.course, "overview") + return clean_dangerous_html( + get_course_about_section(self.request, self.course, "overview") + ) @method_decorator(transaction.non_atomic_requests, name='dispatch') From 670c81f0f2ffb1ef44a9027d7491a5bec59ec52a Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 17 Dec 2025 18:32:43 -0500 Subject: [PATCH 20/21] fix: allow library creation by course creators Prior to this, if ENABLE_ORGANIZATION_STAFF_ACCESS_FOR_CONTENT_LIBRARIES was enabled, we would not return the orgs that someone had course creator rights on, even if ENABLE_CREATOR_GROUP was enabled. (For the moment, we are conflating "can create courses" with "can create libraries" for a given org, even though we should probably eventually split those apart.) --- cms/djangoapps/contentstore/views/course.py | 18 +++- .../views/tests/test_organizations.py | 99 +++++++++++++++++++ 2 files changed, 112 insertions(+), 5 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index f965ebb7aaa7..29e890102e73 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -1843,12 +1843,20 @@ def get_allowed_organizations_for_libraries(user): """ Helper method for returning the list of organizations for which the user is allowed to create libraries. """ + organizations_set = set() + + # This allows org-level staff to create libraries. We should re-evaluate + # whether this is necessary and try to normalize course and library creation + # authorization behavior. if settings.FEATURES.get('ENABLE_ORGANIZATION_STAFF_ACCESS_FOR_CONTENT_LIBRARIES', False): - return get_organizations_for_non_course_creators(user) - elif settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): - return get_organizations(user) - else: - return [] + organizations_set.update(get_organizations_for_non_course_creators(user)) + + # This allows people in the course creator group for an org to create + # libraries, which mimics course behavior. + if settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): + organizations_set.update(get_organizations(user)) + + return sorted(organizations_set) def user_can_create_organizations(user): diff --git a/cms/djangoapps/contentstore/views/tests/test_organizations.py b/cms/djangoapps/contentstore/views/tests/test_organizations.py index cf3a376f3461..d231e3adc507 100644 --- a/cms/djangoapps/contentstore/views/tests/test_organizations.py +++ b/cms/djangoapps/contentstore/views/tests/test_organizations.py @@ -3,12 +3,18 @@ import json +from django.conf import settings from django.test import TestCase +from django.test.utils import override_settings from django.urls import reverse from organizations.api import add_organization +from cms.djangoapps.course_creators.models import CourseCreator +from common.djangoapps.student.roles import OrgStaffRole from common.djangoapps.student.tests.factories import UserFactory +from ..course import get_allowed_organizations_for_libraries + class TestOrganizationListing(TestCase): """Verify Organization listing behavior.""" @@ -32,3 +38,96 @@ def test_organization_list(self): self.assertEqual(response.status_code, 200) org_names = json.loads(response.content.decode('utf-8')) self.assertEqual(org_names, self.org_short_names) + + +class TestOrganizationsForLibraries(TestCase): + """ + Verify who is allowed to create Libraries. + + This uses some low-level implementation details to set up course creator and + org staff data, which should be replaced by API calls. + + The behavior of this call depends on two FEATURES toggles: + + * ENABLE_ORGANIZATION_STAFF_ACCESS_FOR_CONTENT_LIBRARIES + * ENABLE_CREATOR_GROUP + """ + + @classmethod + def setUpTestData(cls): + cls.library_author = UserFactory(is_staff=False) + cls.org_short_names = ["OrgStaffOrg", "CreatorOrg", "RandomOrg"] + cls.orgs = {} + for index, short_name in enumerate(cls.org_short_names): + cls.orgs[short_name] = add_organization(organization_data={ + 'name': 'Test Organization %s' % index, + 'short_name': short_name, + 'description': 'Testing Organization %s Description' % index, + }) + + # Our user is an org staff for OrgStaffOrg + OrgStaffRole("OrgStaffOrg").add_users(cls.library_author) + + # Our user is also a CourseCreator in CreatorOrg + creator = CourseCreator.objects.create( + user=cls.library_author, + state=CourseCreator.GRANTED, + all_organizations=False, + ) + # The following is because course_creators app logic assumes that all + # updates to CourseCreator go through the CourseCreatorAdmin. + # Specifically, CourseCreatorAdmin.save_model() attaches the current + # request.user to the model instance's .admin field, and then the + # course_creator_organizations_changed_callback() signal handler assumes + # creator.admin is present. I think that code could use some judicious + # refactoring, but I'm just writing this test as part of a last-minute + # Ulmo bug fix, and I don't want to add risk by refactoring something as + # critical-path as course_creators as part of this work. + creator.admin = UserFactory(is_staff=True) + creator.organizations.add( + cls.orgs["CreatorOrg"]['id'] + ) + + @override_settings( + FEATURES={ + **settings.FEATURES, + 'ENABLE_ORGANIZATION_STAFF_ACCESS_FOR_CONTENT_LIBRARIES': False, + 'ENABLE_CREATOR_GROUP': False, + } + ) + def test_both_toggles_disabled(self): + allowed_orgs = get_allowed_organizations_for_libraries(self.library_author) + assert allowed_orgs == [] + + @override_settings( + FEATURES={ + **settings.FEATURES, + 'ENABLE_ORGANIZATION_STAFF_ACCESS_FOR_CONTENT_LIBRARIES': True, + 'ENABLE_CREATOR_GROUP': True, + } + ) + def test_both_toggles_enabled(self): + allowed_orgs = get_allowed_organizations_for_libraries(self.library_author) + assert allowed_orgs == ["CreatorOrg", "OrgStaffOrg"] + + @override_settings( + FEATURES={ + **settings.FEATURES, + 'ENABLE_ORGANIZATION_STAFF_ACCESS_FOR_CONTENT_LIBRARIES': True, + 'ENABLE_CREATOR_GROUP': False, + } + ) + def test_org_staff_enabled(self): + allowed_orgs = get_allowed_organizations_for_libraries(self.library_author) + assert allowed_orgs == ["OrgStaffOrg"] + + @override_settings( + FEATURES={ + **settings.FEATURES, + 'ENABLE_ORGANIZATION_STAFF_ACCESS_FOR_CONTENT_LIBRARIES': False, + 'ENABLE_CREATOR_GROUP': True, + } + ) + def test_creator_group_enabled(self): + allowed_orgs = get_allowed_organizations_for_libraries(self.library_author) + assert allowed_orgs == ["CreatorOrg"] From e3084cf4c3752d07edd39ea1642f2f121dc7b04d Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Fri, 19 Dec 2025 10:51:58 -0500 Subject: [PATCH 21/21] build: Don't update common_constraints.txt on re-compilation. Re-compilation and upgrade-package should be able to run without updating the common_constraints.txt file. We do this all the time when backporting fixes to older releases. We shouldn't pull in the latest common_constraints.txt in those cases as they may not be compatible with older releases. --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 92a2e37b9aac..ed6d1f2b7a41 100644 --- a/Makefile +++ b/Makefile @@ -116,7 +116,7 @@ $(COMMON_CONSTRAINTS_TXT): printf "$(COMMON_CONSTRAINTS_TEMP_COMMENT)" | cat - $(@) > temp && mv temp $(@) compile-requirements: export CUSTOM_COMPILE_COMMAND=make upgrade -compile-requirements: pre-requirements $(COMMON_CONSTRAINTS_TXT) ## Re-compile *.in requirements to *.txt +compile-requirements: pre-requirements ## Re-compile *.in requirements to *.txt @# Bootstrapping: Rebuild pip and pip-tools first, and then install them @# so that if there are any failures we'll know now, rather than the next @# time someone tries to use the outputs. @@ -139,7 +139,7 @@ compile-requirements: pre-requirements $(COMMON_CONSTRAINTS_TXT) ## Re-compile * export REBUILD=''; \ done -upgrade: ## update the pip requirements files to use the latest releases satisfying our constraints +upgrade: $(COMMON_CONSTRAINTS_TXT) ## update the pip requirements files to use the latest releases satisfying our constraints $(MAKE) compile-requirements COMPILE_OPTS="--upgrade" upgrade-package: ## update just one package to the latest usable release