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 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/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/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/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/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. diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 453e30e0aad0..29e890102e73 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 = {} @@ -1840,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"] 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/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]]: """ 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): """ 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/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): 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) 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()); }); 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/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index 8d32e4dbc015..d77061d583be 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,9 @@ "revert_changes", "get_backup_task_status", "assign_library_role_to_user", + "user_has_permission_across_lib_authz_systems", + "is_library_backup_task", + "is_library_restore_task", ] @@ -245,7 +246,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 +279,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 +348,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 +766,102 @@ 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 + + +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/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..e72980f6ba0d 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,9 +239,9 @@ 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) + api.publish_component_changes(key, request.user.id) 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/tasks.py b/openedx/core/djangoapps/content_libraries/tasks.py index 93d9fef725ec..bbbf847bfbc7 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 @@ -127,6 +128,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 " @@ -502,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: @@ -519,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) @@ -548,7 +559,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: @@ -579,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_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/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_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. diff --git a/openedx/core/djangoapps/content_libraries/tests/test_tasks.py b/openedx/core/djangoapps/content_libraries/tests/test_tasks.py index 4098b2a8fff9..a200471c00bd 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_tasks.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_tasks.py @@ -1,7 +1,9 @@ """ Unit tests for content libraries Celery tasks """ +from unittest import mock +from django.test import override_settings from ..models import ContentLibrary from .base import ContentLibrariesRestApiTest @@ -13,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() @@ -28,16 +31,26 @@ 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)) + 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() 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) + 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() 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/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') 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" }, 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(): diff --git a/requirements/common_constraints.txt b/requirements/common_constraints.txt index 28ebe29f5cc9..1f3e81f50334 100644 --- a/requirements/common_constraints.txt +++ b/requirements/common_constraints.txt @@ -16,14 +16,14 @@ # 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 # 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..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.29.1 +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-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 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 1f6eb49a2ad3..60e19cd22853 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 @@ -464,6 +465,7 @@ edx-django-utils==8.0.1 # edx-when # enterprise-integrated-channels # event-tracking + # openedx-authz # openedx-events # ora2 # super-csv @@ -825,7 +827,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==0.11.2 +openedx-authz==0.20.0 # via -r requirements/edx/kernel.in openedx-calc==4.0.2 # via -r requirements/edx/kernel.in @@ -854,7 +856,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.1 # via # -c requirements/constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index b3fc16072930..85ac66aed195 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 @@ -746,6 +747,7 @@ edx-django-utils==8.0.1 # edx-when # enterprise-integrated-channels # event-tracking + # openedx-authz # openedx-events # ora2 # super-csv @@ -1375,7 +1377,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==0.11.2 +openedx-authz==0.20.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt @@ -1418,7 +1420,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.1 # via # -c requirements/constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index ac81bd89e8bd..68c115676fe9 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 @@ -552,6 +553,7 @@ edx-django-utils==8.0.1 # edx-when # enterprise-integrated-channels # event-tracking + # openedx-authz # openedx-events # ora2 # super-csv @@ -1002,7 +1004,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==0.11.2 +openedx-authz==0.20.0 # via -r requirements/edx/base.txt openedx-calc==4.0.2 # via -r requirements/edx/base.txt @@ -1032,7 +1034,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.1 # via # -c requirements/constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 5cfe412a7fbd..f58ef16f9e31 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 @@ -574,6 +575,7 @@ edx-django-utils==8.0.1 # edx-when # enterprise-integrated-channels # event-tracking + # openedx-authz # openedx-events # ora2 # super-csv @@ -1048,7 +1050,7 @@ openedx-atlas==0.7.0 # enterprise-integrated-channels # openedx-authz # openedx-forum -openedx-authz==0.11.2 +openedx-authz==0.20.0 # via -r requirements/edx/base.txt openedx-calc==4.0.2 # via -r requirements/edx/base.txt @@ -1078,7 +1080,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.1 # via # -c requirements/constraints.txt # -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