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"],
+ "