From dc19c0906a01d83078a7e0d0b6ceda556d637a20 Mon Sep 17 00:00:00 2001 From: Kevyn Suarez Date: Mon, 27 Jul 2026 19:22:05 -0500 Subject: [PATCH 1/6] feat: support role assignment for scopes that don't exist yet --- openedx_authz/handlers.py | 34 +++++ .../migrations/0010_scope_external_key.py | 18 +++ openedx_authz/models/core.py | 7 + openedx_authz/models/scopes.py | 51 +++++++- .../tests/integration/test_models.py | 120 +++++++++++++++++- 5 files changed, 225 insertions(+), 5 deletions(-) create mode 100644 openedx_authz/migrations/0010_scope_external_key.py diff --git a/openedx_authz/handlers.py b/openedx_authz/handlers.py index be6629c0..f404c51a 100644 --- a/openedx_authz/handlers.py +++ b/openedx_authz/handlers.py @@ -20,6 +20,7 @@ from openedx_authz.engine.utils import run_course_authoring_migration from openedx_authz.models.authz_migration import MigrationType, ScopeType from openedx_authz.models.core import ExtendedCasbinRule, RoleAssignmentAudit +from openedx_authz.models.scopes import ContentLibrary, ContentLibraryScope, CourseOverview, CourseScope from openedx_authz.models.subjects import UserSubject try: @@ -141,6 +142,39 @@ def handle_org_waffle_flag_change(sender, instance, **kwargs) -> None: post_save.connect(handle_org_waffle_flag_change, sender=WaffleFlagOrgOverrideModel) +def backfill_course_scope(sender, instance, **kwargs): # pylint: disable=unused-argument + """ + Link a pending CourseScope to the CourseOverview that was just created for it. + + CourseScope.get_or_create_for_external_key() (openedx_authz/models/scopes.py) allows + assigning a role for a course key before its CourseOverview exists, leaving + course_overview null and external_key set to the course id. This backfills that FK + once the course shows up, e.g. after a course rerun finishes cloning. + """ + CourseScope.objects.filter( + external_key=str(instance.id), course_overview__isnull=True + ).update(course_overview=instance) + + +def backfill_content_library_scope(sender, instance, **kwargs): # pylint: disable=unused-argument + """ + Link a pending ContentLibraryScope to the ContentLibrary that was just created for it. + + See backfill_course_scope() above; same idea for content libraries. + """ + ContentLibraryScope.objects.filter( + external_key=str(instance.library_key), content_library__isnull=True + ).update(content_library=instance) + + +# Only register the handlers if the models are available (i.e., running in Open edX) +if CourseOverview is not None: + post_save.connect(backfill_course_scope, sender=CourseOverview) + +if ContentLibrary is not None: + post_save.connect(backfill_content_library_scope, sender=ContentLibrary) + + # Match ``WaffleFlagCourseOverrideModel.OVERRIDE_CHOICES`` / ``override_value`` in edx-platform: # the flag is effectively forced on only when the row is enabled and ``override_choice`` is "on". WAFFLE_OVERRIDE_FORCE_ON = "on" diff --git a/openedx_authz/migrations/0010_scope_external_key.py b/openedx_authz/migrations/0010_scope_external_key.py new file mode 100644 index 00000000..c6cd332e --- /dev/null +++ b/openedx_authz/migrations/0010_scope_external_key.py @@ -0,0 +1,18 @@ +# Generated by Django 5.2.16 on 2026-07-28 00:04 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('openedx_authz', '0009_roleassignmentaudit'), + ] + + operations = [ + migrations.AddField( + model_name='scope', + name='external_key', + field=models.CharField(blank=True, db_index=True, max_length=255, null=True, unique=True), + ), + ] diff --git a/openedx_authz/models/core.py b/openedx_authz/models/core.py index 7a2584ef..7398b01b 100644 --- a/openedx_authz/models/core.py +++ b/openedx_authz/models/core.py @@ -117,6 +117,13 @@ class Scope(BaseRegistryModel): objects = ScopeManager() + # Canonical string form of the scope's key (e.g. a course-v1 course id), set on creation + # regardless of whether the backing object (CourseOverview, ContentLibrary, ...) exists yet. + # This is the only way to find a scope back again when its FK to that object is still null + # (see get_or_create_for_external_key() in openedx_authz/models/scopes.py) so it can be + # linked up once the object is created (see openedx_authz/handlers.py backfill receivers). + external_key = models.CharField(max_length=255, null=True, blank=True, unique=True, db_index=True) + class Meta: abstract = False diff --git a/openedx_authz/models/scopes.py b/openedx_authz/models/scopes.py index ba28bda4..e1509d5d 100644 --- a/openedx_authz/models/scopes.py +++ b/openedx_authz/models/scopes.py @@ -84,6 +84,12 @@ class ContentLibraryScope(Scope): def get_or_create_for_external_key(cls, scope) -> "ContentLibraryScope": """Get or create a ContentLibraryScope for the given external key. + The backing ContentLibrary need not exist yet (e.g. it may be created + later as part of an in-progress operation): the scope is created with + ``content_library=None`` in that case and gets linked up automatically + once a ContentLibrary with a matching key is saved (see the backfill + signal receiver in openedx_authz/handlers.py). + Args: scope: ScopeData object with an external_key attribute containing a LibraryLocatorV2-compatible string. @@ -93,8 +99,23 @@ def get_or_create_for_external_key(cls, scope) -> "ContentLibraryScope": or None if the scope is a glob pattern (contains wildcard). """ library_key = LibraryLocatorV2.from_string(scope.external_key) - content_library = ContentLibrary.objects.get_by_key(library_key) - scope, _ = cls.objects.get_or_create(content_library=content_library) + try: + content_library = ContentLibrary.objects.get_by_key(library_key) + except ContentLibrary.DoesNotExist: + # The library doesn't exist yet: key the row by its external_key so it can be + # found and linked up later by the backfill signal in openedx_authz/handlers.py. + scope, _ = cls.objects.get_or_create(external_key=str(library_key)) + return scope + + # Look up by the FK first (as before this change) so scopes created before the + # external_key field existed are reused rather than duplicated. + scope, created = cls.objects.get_or_create( + content_library=content_library, + defaults={"external_key": str(library_key)}, + ) + if not created and not scope.external_key: + scope.external_key = str(library_key) + scope.save(update_fields=["external_key"]) return scope @@ -128,6 +149,13 @@ class CourseScope(Scope): def get_or_create_for_external_key(cls, scope) -> "CourseScope": """Get or create a CourseScope for the given external key. + The backing CourseOverview need not exist yet (e.g. during a course + rerun, the destination course id is known before the course is + cloned): the scope is created with ``course_overview=None`` in that + case and gets linked up automatically once a CourseOverview with a + matching id is saved (see the backfill signal receiver in + openedx_authz/handlers.py). + Args: scope: ScopeData object with an external_key attribute containing a CourseKey string. @@ -137,6 +165,21 @@ def get_or_create_for_external_key(cls, scope) -> "CourseScope": or None if the scope is a glob pattern (contains wildcard). """ course_key = CourseKey.from_string(scope.external_key) - course_overview = CourseOverview.get_from_id(course_key) - scope, _ = cls.objects.get_or_create(course_overview=course_overview) + try: + course_overview = CourseOverview.get_from_id(course_key) + except CourseOverview.DoesNotExist: + # The course doesn't exist yet: key the row by its external_key so it can be + # found and linked up later by the backfill signal in openedx_authz/handlers.py. + scope, _ = cls.objects.get_or_create(external_key=str(course_key)) + return scope + + # Look up by the FK first (as before this change) so scopes created before the + # external_key field existed are reused rather than duplicated. + scope, created = cls.objects.get_or_create( + course_overview=course_overview, + defaults={"external_key": str(course_key)}, + ) + if not created and not scope.external_key: + scope.external_key = str(course_key) + scope.save(update_fields=["external_key"]) return scope diff --git a/openedx_authz/tests/integration/test_models.py b/openedx_authz/tests/integration/test_models.py index b7d5a4f3..27b0a896 100644 --- a/openedx_authz/tests/integration/test_models.py +++ b/openedx_authz/tests/integration/test_models.py @@ -29,12 +29,19 @@ from organizations.api import ensure_organization from organizations.models import Organization -from openedx_authz.api.data import ContentLibraryData, RoleData, SubjectData, UserData +from opaque_keys.edx.locator import CourseLocator +from openedx.core.djangoapps.content.course_overviews.tests.factories import ( # pylint: disable=import-error + CourseOverviewFactory, +) + +from openedx_authz.api.data import ContentLibraryData, CourseOverviewData, RoleData, SubjectData, UserData from openedx_authz.api.roles import assign_role_to_subject_in_scope from openedx_authz.engine.enforcer import AuthzEnforcer from openedx_authz.models import ( ContentLibrary, ContentLibraryScope, + CourseOverview, + CourseScope, ExtendedCasbinRule, Scope, Subject, @@ -1356,3 +1363,114 @@ def test_extended_casbin_rule_with_null_subject_deletion(self): self.assertFalse(CasbinRule.objects.filter(id=casbin_rule_id).exists()) self.assertTrue(Scope.objects.filter(id=scope_id).exists()) + + +@pytest.mark.integration +@override_settings(OPENEDX_AUTHZ_CONTENT_LIBRARY_MODEL="content_libraries.ContentLibrary") +class TestScopeForPendingObjects(TestCase): + """Test cases for scopes whose backing object doesn't exist yet. + + Covers https://github.com/openedx/openedx-authz/issues/352: role assignment must succeed + for a course/library key before the CourseOverview/ContentLibrary behind it is created (e.g. + during a course rerun, the destination course id is known before the course is cloned), and + the Scope must link up to the real object once it's created (see the backfill signal + receivers in openedx_authz/handlers.py). + """ + + def test_course_scope_created_without_course_overview(self): + """Assigning a role for a course that doesn't exist yet must not raise.""" + course_key = CourseLocator("PendingOrg", "PendingCourse", "2024_T1") + scope_data = CourseOverviewData(external_key=str(course_key)) + + scope = Scope.objects.get_or_create_for_external_key(scope_data) + + self.assertIsInstance(scope, CourseScope) + self.assertIsNone(scope.course_overview) + self.assertEqual(scope.external_key, str(course_key)) + + def test_course_scope_backfills_when_course_overview_created(self): + """Creating the CourseOverview later links up the previously-pending CourseScope.""" + course_key = CourseLocator("PendingOrg2", "PendingCourse2", "2024_T1") + scope_data = CourseOverviewData(external_key=str(course_key)) + scope = Scope.objects.get_or_create_for_external_key(scope_data) + self.assertIsNone(scope.course_overview) + + course_overview = CourseOverviewFactory(id=course_key) + + scope.refresh_from_db() + self.assertEqual(scope.course_overview, course_overview) + + def test_course_scope_get_or_create_is_idempotent_while_pending(self): + """Calling get_or_create_for_external_key twice before the course exists reuses the same row.""" + course_key = CourseLocator("PendingOrg3", "PendingCourse3", "2024_T1") + scope_data = CourseOverviewData(external_key=str(course_key)) + + scope1 = Scope.objects.get_or_create_for_external_key(scope_data) + scope2 = Scope.objects.get_or_create_for_external_key(scope_data) + + self.assertEqual(scope1.id, scope2.id) + self.assertEqual(CourseScope.objects.filter(external_key=str(course_key)).count(), 1) + + def test_assign_role_succeeds_for_course_that_does_not_exist_yet(self): + """The public API assign_role_to_subject_in_scope must not crash for a not-yet-existing course.""" + test_username = "pending_course_instructor" + User.objects.create_user(username=test_username) + course_key = CourseLocator("PendingOrg4", "PendingCourse4", "2024_T1") + + subject_data = UserData(external_key=test_username) + role_data = RoleData(external_key="instructor") + scope_data = CourseOverviewData(external_key=str(course_key)) + + result = assign_role_to_subject_in_scope(subject_data, role_data, scope_data) + + self.assertTrue(result) + scope = CourseScope.objects.get(external_key=str(course_key)) + self.assertIsNone(scope.course_overview) + + # The course gets created later (e.g. once course rerun finishes cloning). + course_overview = CourseOverviewFactory(id=course_key) + scope.refresh_from_db() + self.assertEqual(scope.course_overview, course_overview) + + def test_content_library_scope_created_without_content_library(self): + """Assigning a role for a library that doesn't exist yet must not raise.""" + library_key = "lib:PendingOrg:pendinglib" + scope_data = ContentLibraryData(external_key=library_key) + + scope = Scope.objects.get_or_create_for_external_key(scope_data) + + self.assertIsInstance(scope, ContentLibraryScope) + self.assertIsNone(scope.content_library) + self.assertEqual(scope.external_key, library_key) + + def test_content_library_scope_backfills_when_library_created(self): + """Creating the ContentLibrary later links up the previously-pending ContentLibraryScope.""" + org_short_name = "PendingLibOrg" + slug = "pendinglib2" + library_key = f"lib:{org_short_name}:{slug}" + scope_data = ContentLibraryData(external_key=library_key) + scope = Scope.objects.get_or_create_for_external_key(scope_data) + self.assertIsNone(scope.content_library) + + _, _, content_library = create_test_library(org_short_name=org_short_name, slug=slug) + + scope.refresh_from_db() + self.assertEqual(scope.content_library, content_library) + + def test_scope_created_before_external_key_field_is_reused_not_duplicated(self): + """A Scope row created before this fix (FK set, no external_key) isn't duplicated on reassignment. + + Simulates a pre-existing row the way get_or_create_for_external_key used to create it: + keyed only by the FK, with no external_key. + """ + course_key = CourseLocator("LegacyOrg", "LegacyCourse", "2024_T1") + course_overview = CourseOverviewFactory(id=course_key) + legacy_scope = CourseScope.objects.create(course_overview=course_overview) + self.assertIsNone(legacy_scope.external_key) + + scope_data = CourseOverviewData(external_key=str(course_key)) + scope = Scope.objects.get_or_create_for_external_key(scope_data) + + self.assertEqual(scope.id, legacy_scope.id) + self.assertEqual(scope.external_key, str(course_key)) + self.assertEqual(CourseScope.objects.filter(course_overview=course_overview).count(), 1) From 5e7d9e12967eacfecf3cf96a5e6dfeba095e074a Mon Sep 17 00:00:00 2001 From: Kevyn Suarez Date: Wed, 29 Jul 2026 12:38:37 -0500 Subject: [PATCH 2/6] fix: ci linting warning --- openedx_authz/tests/integration/test_models.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openedx_authz/tests/integration/test_models.py b/openedx_authz/tests/integration/test_models.py index 27b0a896..8e50df63 100644 --- a/openedx_authz/tests/integration/test_models.py +++ b/openedx_authz/tests/integration/test_models.py @@ -40,7 +40,6 @@ from openedx_authz.models import ( ContentLibrary, ContentLibraryScope, - CourseOverview, CourseScope, ExtendedCasbinRule, Scope, From c8c8ebe163dfef413fe24b8fb7122aa9a634de89 Mon Sep 17 00:00:00 2001 From: Kevyn Suarez Date: Wed, 29 Jul 2026 13:17:04 -0500 Subject: [PATCH 3/6] fix: last ci check --- openedx_authz/tests/integration/test_models.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/openedx_authz/tests/integration/test_models.py b/openedx_authz/tests/integration/test_models.py index 8e50df63..13ffc664 100644 --- a/openedx_authz/tests/integration/test_models.py +++ b/openedx_authz/tests/integration/test_models.py @@ -26,13 +26,12 @@ from django.contrib.auth import get_user_model from django.db import IntegrityError from django.test import TestCase, override_settings -from organizations.api import ensure_organization -from organizations.models import Organization - from opaque_keys.edx.locator import CourseLocator from openedx.core.djangoapps.content.course_overviews.tests.factories import ( # pylint: disable=import-error CourseOverviewFactory, ) +from organizations.api import ensure_organization +from organizations.models import Organization from openedx_authz.api.data import ContentLibraryData, CourseOverviewData, RoleData, SubjectData, UserData from openedx_authz.api.roles import assign_role_to_subject_in_scope From c00ffa2f3ea443f4146b677d4b338936c7b5a6f7 Mon Sep 17 00:00:00 2001 From: Kevyn Suarez Date: Wed, 29 Jul 2026 14:48:12 -0500 Subject: [PATCH 4/6] fix: address review feedback on pending-scope handling - Drop redundant db_index on Scope.external_key (unique already indexes it). - Reuse ScopeData.get_object() instead of duplicating the DoesNotExist handling in CourseScope/ContentLibraryScope.get_or_create_for_external_key(). - Add CourseScope.link_pending_scope()/ContentLibraryScope.link_pending_scope() so the handlers.py signal receivers no longer touch the model querysets directly. Addresses review comments from mariajgrimaldi on #369. --- openedx_authz/handlers.py | 16 ++-- .../migrations/0010_scope_external_key.py | 4 +- openedx_authz/models/core.py | 2 +- openedx_authz/models/scopes.py | 86 ++++++++++++------- 4 files changed, 63 insertions(+), 45 deletions(-) diff --git a/openedx_authz/handlers.py b/openedx_authz/handlers.py index f404c51a..7e0ba242 100644 --- a/openedx_authz/handlers.py +++ b/openedx_authz/handlers.py @@ -146,14 +146,12 @@ def backfill_course_scope(sender, instance, **kwargs): # pylint: disable=unused """ Link a pending CourseScope to the CourseOverview that was just created for it. - CourseScope.get_or_create_for_external_key() (openedx_authz/models/scopes.py) allows - assigning a role for a course key before its CourseOverview exists, leaving - course_overview null and external_key set to the course id. This backfills that FK - once the course shows up, e.g. after a course rerun finishes cloning. + get_or_create_for_external_key() (openedx_authz/models/scopes.py) allows assigning a + role for a course key before its CourseOverview exists, leaving course_overview null. + This backfills that FK once the course shows up, e.g. after a course rerun finishes + cloning. See CourseScope.link_pending_scope() for the actual lookup/update. """ - CourseScope.objects.filter( - external_key=str(instance.id), course_overview__isnull=True - ).update(course_overview=instance) + CourseScope.link_pending_scope(instance) def backfill_content_library_scope(sender, instance, **kwargs): # pylint: disable=unused-argument @@ -162,9 +160,7 @@ def backfill_content_library_scope(sender, instance, **kwargs): # pylint: disab See backfill_course_scope() above; same idea for content libraries. """ - ContentLibraryScope.objects.filter( - external_key=str(instance.library_key), content_library__isnull=True - ).update(content_library=instance) + ContentLibraryScope.link_pending_scope(instance) # Only register the handlers if the models are available (i.e., running in Open edX) diff --git a/openedx_authz/migrations/0010_scope_external_key.py b/openedx_authz/migrations/0010_scope_external_key.py index c6cd332e..2ec32a0c 100644 --- a/openedx_authz/migrations/0010_scope_external_key.py +++ b/openedx_authz/migrations/0010_scope_external_key.py @@ -1,4 +1,4 @@ -# Generated by Django 5.2.16 on 2026-07-28 00:04 +# Generated by Django 5.2.16 on 2026-07-29 19:03 from django.db import migrations, models @@ -13,6 +13,6 @@ class Migration(migrations.Migration): migrations.AddField( model_name='scope', name='external_key', - field=models.CharField(blank=True, db_index=True, max_length=255, null=True, unique=True), + field=models.CharField(blank=True, max_length=255, null=True, unique=True), ), ] diff --git a/openedx_authz/models/core.py b/openedx_authz/models/core.py index 7398b01b..cd3b9b97 100644 --- a/openedx_authz/models/core.py +++ b/openedx_authz/models/core.py @@ -122,7 +122,7 @@ class Scope(BaseRegistryModel): # This is the only way to find a scope back again when its FK to that object is still null # (see get_or_create_for_external_key() in openedx_authz/models/scopes.py) so it can be # linked up once the object is created (see openedx_authz/handlers.py backfill receivers). - external_key = models.CharField(max_length=255, null=True, blank=True, unique=True, db_index=True) + external_key = models.CharField(max_length=255, null=True, blank=True, unique=True) class Meta: abstract = False diff --git a/openedx_authz/models/scopes.py b/openedx_authz/models/scopes.py index e1509d5d..7b2e933a 100644 --- a/openedx_authz/models/scopes.py +++ b/openedx_authz/models/scopes.py @@ -8,8 +8,6 @@ from django.apps import apps from django.conf import settings from django.db import models -from opaque_keys.edx.keys import CourseKey -from opaque_keys.edx.locator import LibraryLocatorV2 from openedx_authz.models.core import Scope @@ -87,8 +85,8 @@ def get_or_create_for_external_key(cls, scope) -> "ContentLibraryScope": The backing ContentLibrary need not exist yet (e.g. it may be created later as part of an in-progress operation): the scope is created with ``content_library=None`` in that case and gets linked up automatically - once a ContentLibrary with a matching key is saved (see the backfill - signal receiver in openedx_authz/handlers.py). + once a ContentLibrary with a matching key is saved (see link_pending_scope() + and the backfill signal receiver in openedx_authz/handlers.py). Args: scope: ScopeData object with an external_key attribute containing @@ -98,25 +96,37 @@ def get_or_create_for_external_key(cls, scope) -> "ContentLibraryScope": ContentLibraryScope: The Scope instance for the given ContentLibrary, or None if the scope is a glob pattern (contains wildcard). """ - library_key = LibraryLocatorV2.from_string(scope.external_key) - try: - content_library = ContentLibrary.objects.get_by_key(library_key) - except ContentLibrary.DoesNotExist: + content_library = scope.get_object() + if content_library is None: # The library doesn't exist yet: key the row by its external_key so it can be - # found and linked up later by the backfill signal in openedx_authz/handlers.py. - scope, _ = cls.objects.get_or_create(external_key=str(library_key)) - return scope + # found and linked up later by link_pending_scope(). + row, _ = cls.objects.get_or_create(external_key=scope.external_key) + return row # Look up by the FK first (as before this change) so scopes created before the # external_key field existed are reused rather than duplicated. - scope, created = cls.objects.get_or_create( + row, created = cls.objects.get_or_create( content_library=content_library, - defaults={"external_key": str(library_key)}, + defaults={"external_key": scope.external_key}, ) - if not created and not scope.external_key: - scope.external_key = str(library_key) - scope.save(update_fields=["external_key"]) - return scope + if not created and not row.external_key: + row.external_key = scope.external_key + row.save(update_fields=["external_key"]) + return row + + @classmethod + def link_pending_scope(cls, content_library) -> None: + """Link a pending ContentLibraryScope to the ContentLibrary that was just created for it. + + Called from the ContentLibrary post_save signal receiver in openedx_authz/handlers.py + once a library with a matching external_key shows up. + + Args: + content_library: The ContentLibrary instance that was just saved. + """ + cls.objects.filter( + external_key=str(content_library.library_key), content_library__isnull=True + ).update(content_library=content_library) class CourseScope(Scope): @@ -153,8 +163,8 @@ def get_or_create_for_external_key(cls, scope) -> "CourseScope": rerun, the destination course id is known before the course is cloned): the scope is created with ``course_overview=None`` in that case and gets linked up automatically once a CourseOverview with a - matching id is saved (see the backfill signal receiver in - openedx_authz/handlers.py). + matching id is saved (see link_pending_scope() and the backfill + signal receiver in openedx_authz/handlers.py). Args: scope: ScopeData object with an external_key attribute containing @@ -164,22 +174,34 @@ def get_or_create_for_external_key(cls, scope) -> "CourseScope": CourseScope: The Scope instance for the given CourseOverview, or None if the scope is a glob pattern (contains wildcard). """ - course_key = CourseKey.from_string(scope.external_key) - try: - course_overview = CourseOverview.get_from_id(course_key) - except CourseOverview.DoesNotExist: + course_overview = scope.get_object() + if course_overview is None: # The course doesn't exist yet: key the row by its external_key so it can be - # found and linked up later by the backfill signal in openedx_authz/handlers.py. - scope, _ = cls.objects.get_or_create(external_key=str(course_key)) - return scope + # found and linked up later by link_pending_scope(). + row, _ = cls.objects.get_or_create(external_key=scope.external_key) + return row # Look up by the FK first (as before this change) so scopes created before the # external_key field existed are reused rather than duplicated. - scope, created = cls.objects.get_or_create( + row, created = cls.objects.get_or_create( course_overview=course_overview, - defaults={"external_key": str(course_key)}, + defaults={"external_key": scope.external_key}, ) - if not created and not scope.external_key: - scope.external_key = str(course_key) - scope.save(update_fields=["external_key"]) - return scope + if not created and not row.external_key: + row.external_key = scope.external_key + row.save(update_fields=["external_key"]) + return row + + @classmethod + def link_pending_scope(cls, course_overview) -> None: + """Link a pending CourseScope to the CourseOverview that was just created for it. + + Called from the CourseOverview post_save signal receiver in openedx_authz/handlers.py + once a course with a matching external_key shows up. + + Args: + course_overview: The CourseOverview instance that was just saved. + """ + cls.objects.filter( + external_key=str(course_overview.id), course_overview__isnull=True + ).update(course_overview=course_overview) From 0a57902fcf3f60848744b20b9209d55d3cabc82d Mon Sep 17 00:00:00 2001 From: Kevyn Suarez Date: Wed, 29 Jul 2026 17:06:19 -0500 Subject: [PATCH 5/6] fix: make pending-scope backfill signals crash-safe The backfill_course_scope/backfill_content_library_scope post_save receivers ran unconditionally on every CourseOverview/ContentLibrary save, including unrelated fixtures elsewhere in the suite. Wrap them in a try/except plus their own savepoint (transaction.atomic()) so a failure to link a pending scope never breaks the caller's save() or poisons its transaction. Also fix the ContentLibrary test stub's get_by_key(): it only set locator, leaving org/slug unset, so library_key crashed with AttributeError as soon as anything (now including ScopeData.get_object()'s canonical-key check) touched it. It now populates org/slug like the real manager does. Fixes the 6 test failures from CI on this PR. --- openedx_authz/handlers.py | 24 +++++++++++++++++++++--- openedx_authz/tests/stubs/models.py | 14 +++++++++----- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/openedx_authz/handlers.py b/openedx_authz/handlers.py index 7e0ba242..1f5480ac 100644 --- a/openedx_authz/handlers.py +++ b/openedx_authz/handlers.py @@ -11,6 +11,7 @@ from casbin_adapter.models import CasbinRule from django.conf import settings +from django.db import transaction from django.db.models.signals import post_delete, post_save from django.dispatch import receiver from openedx_events.authz.signals import ROLE_ASSIGNMENT_CREATED, ROLE_ASSIGNMENT_DELETED @@ -150,17 +151,34 @@ def backfill_course_scope(sender, instance, **kwargs): # pylint: disable=unused role for a course key before its CourseOverview exists, leaving course_overview null. This backfills that FK once the course shows up, e.g. after a course rerun finishes cloning. See CourseScope.link_pending_scope() for the actual lookup/update. + + This runs on every CourseOverview save, so errors are logged rather than raised: + a problem backfilling a pending scope must not break the caller's save(). The query + runs in its own savepoint so a failure doesn't poison the caller's transaction. """ - CourseScope.link_pending_scope(instance) + try: + with transaction.atomic(): + CourseScope.link_pending_scope(instance) + except Exception as exc: # pylint: disable=broad-exception-caught + logger.exception( + "Error linking pending CourseScope to CourseOverview %s", instance.pk, exc_info=exc, + ) def backfill_content_library_scope(sender, instance, **kwargs): # pylint: disable=unused-argument """ Link a pending ContentLibraryScope to the ContentLibrary that was just created for it. - See backfill_course_scope() above; same idea for content libraries. + See backfill_course_scope() above; same idea for content libraries, including the + broad exception handling and the isolating savepoint. """ - ContentLibraryScope.link_pending_scope(instance) + try: + with transaction.atomic(): + ContentLibraryScope.link_pending_scope(instance) + except Exception as exc: # pylint: disable=broad-exception-caught + logger.exception( + "Error linking pending ContentLibraryScope to ContentLibrary %s", instance.pk, exc_info=exc, + ) # Only register the handlers if the models are available (i.e., running in Open edX) diff --git a/openedx_authz/tests/stubs/models.py b/openedx_authz/tests/stubs/models.py index 8bbe8b42..e32fc469 100644 --- a/openedx_authz/tests/stubs/models.py +++ b/openedx_authz/tests/stubs/models.py @@ -39,6 +39,9 @@ class ContentLibraryManager(models.Manager): def get_by_key(self, library_key): """Get or create a ContentLibrary by its library key. + Populates org/slug from the parsed key (like the real manager, which always + returns a fully-formed library), so the library_key property never crashes. + Args: library_key: The library key to look up. @@ -47,11 +50,12 @@ def get_by_key(self, library_key): """ if library_key is None: raise ValueError("library_key must not be None") - try: - key = str(LibraryLocatorV2.from_string(str(library_key))) - except Exception: # pylint: disable=broad-exception-caught - key = str(library_key) - obj, _ = self.get_or_create(locator=key) + parsed_key = LibraryLocatorV2.from_string(str(library_key)) + org, _ = Organization.objects.get_or_create(short_name=parsed_key.org) + obj, _ = self.get_or_create( + locator=str(parsed_key), + defaults={"org": org, "slug": parsed_key.slug}, + ) return obj From f03d4867432dcd9b91c6f67d9c4ca83bae6d8bc5 Mon Sep 17 00:00:00 2001 From: Kevyn Suarez Date: Wed, 29 Jul 2026 18:54:22 -0500 Subject: [PATCH 6/6] refactor: deduplicate pending-scope logic between CourseScope/ContentLibraryScope get_or_create_for_external_key() and link_pending_scope() were nearly identical between the two subclasses, differing only in the FK field name and how to compute external_key from the linked object. Move both methods to the base Scope class, parameterized by a LINKED_OBJECT_FIELD class attribute and an external_key_for_object() hook each subclass implements. Also restores the "scope" variable name (was "row") and fixes the stale "or None if glob pattern" return docstring, which described ScopeManager's dispatch behavior, not this method's. Addresses review comments from BryanttV on #369. --- openedx_authz/models/core.py | 81 +++++++++++++++++++++++-- openedx_authz/models/scopes.py | 105 +++------------------------------ 2 files changed, 85 insertions(+), 101 deletions(-) diff --git a/openedx_authz/models/core.py b/openedx_authz/models/core.py index cd3b9b97..9b4491da 100644 --- a/openedx_authz/models/core.py +++ b/openedx_authz/models/core.py @@ -111,22 +111,95 @@ class Scope(BaseRegistryModel): This model can be extended to represent different types of scopes, such as courses or content libraries. - Subclasses should define a NAMESPACE class attribute (e.g., 'lib' for content libraries) - and implement get_or_create_for_external_key() classmethod. + Subclasses should define a NAMESPACE class attribute (e.g., 'lib' for content libraries), + a LINKED_OBJECT_FIELD naming their FK to the backing object (e.g. 'content_library'), and + implement external_key_for_object(). """ objects = ScopeManager() + # Name of the subclass's FK field pointing at its backing object (e.g. "content_library", + # "course_overview"), used by get_or_create_for_external_key()/link_pending_scope() below + # to work generically across scope types. + LINKED_OBJECT_FIELD: ClassVar[str] = None + # Canonical string form of the scope's key (e.g. a course-v1 course id), set on creation # regardless of whether the backing object (CourseOverview, ContentLibrary, ...) exists yet. # This is the only way to find a scope back again when its FK to that object is still null - # (see get_or_create_for_external_key() in openedx_authz/models/scopes.py) so it can be - # linked up once the object is created (see openedx_authz/handlers.py backfill receivers). + # so it can be linked up once the object is created (see link_pending_scope() below and the + # backfill signal receivers in openedx_authz/handlers.py). external_key = models.CharField(max_length=255, null=True, blank=True, unique=True) class Meta: abstract = False + @classmethod + def external_key_for_object(cls, linked_object) -> str: + """Return the canonical external_key string for one of this scope's backing objects. + + Subclasses must override this to match how their ScopeData computes external_key + (e.g. the course id, or the library key). + + Args: + linked_object: An instance of the model named by LINKED_OBJECT_FIELD. + + Returns: + str: The canonical external_key for that instance. + """ + raise NotImplementedError + + @classmethod + def get_or_create_for_external_key(cls, scope) -> "Scope": + """Get or create a scope instance for the given external key. + + The backing object (CourseOverview, ContentLibrary, ...) need not exist yet (e.g. + during a course rerun, the destination course id is known before the course is + cloned): the scope is created with its LINKED_OBJECT_FIELD left ``None`` in that + case, and gets linked up automatically once a matching object is saved (see + link_pending_scope() below and the backfill signal receivers in + openedx_authz/handlers.py). + + Args: + scope: ScopeData object with an external_key attribute. + + Returns: + Scope: The (possibly newly created) scope instance for this subclass. + """ + external_key = scope.external_key + linked_object = scope.get_object() + if linked_object is None: + # The object doesn't exist yet: key the row by its external_key so it can be + # found and linked up later by link_pending_scope(). + scope, _ = cls.objects.get_or_create(external_key=external_key) + return scope + + # Look up by the FK first (as before the external_key field existed) so scopes + # created before this change are reused rather than duplicated. + scope, created = cls.objects.get_or_create( + **{cls.LINKED_OBJECT_FIELD: linked_object}, + defaults={"external_key": external_key}, + ) + if not created and not scope.external_key: + scope.external_key = external_key + scope.save(update_fields=["external_key"]) + return scope + + @classmethod + def link_pending_scope(cls, linked_object) -> None: + """Link a pending scope to the object that was just created for it. + + Called from the relevant post_save signal receiver in openedx_authz/handlers.py + once an object with a matching external_key shows up. + + Args: + linked_object: The model instance (CourseOverview, ContentLibrary, ...) that + was just saved. + """ + cls.objects.filter( + external_key=cls.external_key_for_object(linked_object), + **{f"{cls.LINKED_OBJECT_FIELD}__isnull": True}, + ).update(**{cls.LINKED_OBJECT_FIELD: linked_object}) + class Subject(BaseRegistryModel): """Model representing a subject in the authorization system. diff --git a/openedx_authz/models/scopes.py b/openedx_authz/models/scopes.py index 7b2e933a..11d110bc 100644 --- a/openedx_authz/models/scopes.py +++ b/openedx_authz/models/scopes.py @@ -59,6 +59,7 @@ class ContentLibraryScope(Scope): """ NAMESPACE = "lib" + LINKED_OBJECT_FIELD = "content_library" # Link to the actual content library, if applicable. In other cases, this could be null. # Piggybacking on the existing ContentLibrary model to keep the ExtendedCasbinRule up to date @@ -79,54 +80,9 @@ class ContentLibraryScope(Scope): ) @classmethod - def get_or_create_for_external_key(cls, scope) -> "ContentLibraryScope": - """Get or create a ContentLibraryScope for the given external key. - - The backing ContentLibrary need not exist yet (e.g. it may be created - later as part of an in-progress operation): the scope is created with - ``content_library=None`` in that case and gets linked up automatically - once a ContentLibrary with a matching key is saved (see link_pending_scope() - and the backfill signal receiver in openedx_authz/handlers.py). - - Args: - scope: ScopeData object with an external_key attribute containing - a LibraryLocatorV2-compatible string. - - Returns: - ContentLibraryScope: The Scope instance for the given ContentLibrary, - or None if the scope is a glob pattern (contains wildcard). - """ - content_library = scope.get_object() - if content_library is None: - # The library doesn't exist yet: key the row by its external_key so it can be - # found and linked up later by link_pending_scope(). - row, _ = cls.objects.get_or_create(external_key=scope.external_key) - return row - - # Look up by the FK first (as before this change) so scopes created before the - # external_key field existed are reused rather than duplicated. - row, created = cls.objects.get_or_create( - content_library=content_library, - defaults={"external_key": scope.external_key}, - ) - if not created and not row.external_key: - row.external_key = scope.external_key - row.save(update_fields=["external_key"]) - return row - - @classmethod - def link_pending_scope(cls, content_library) -> None: - """Link a pending ContentLibraryScope to the ContentLibrary that was just created for it. - - Called from the ContentLibrary post_save signal receiver in openedx_authz/handlers.py - once a library with a matching external_key shows up. - - Args: - content_library: The ContentLibrary instance that was just saved. - """ - cls.objects.filter( - external_key=str(content_library.library_key), content_library__isnull=True - ).update(content_library=content_library) + def external_key_for_object(cls, linked_object) -> str: + """Return the canonical external_key string for a ContentLibrary instance.""" + return str(linked_object.library_key) class CourseScope(Scope): @@ -136,6 +92,7 @@ class CourseScope(Scope): """ NAMESPACE = "course-v1" + LINKED_OBJECT_FIELD = "course_overview" # Link to the actual course, if applicable. In other cases, this could be null. # Piggybacking on the existing CourseOverview model to keep the ExtendedCasbinRule up to date @@ -156,52 +113,6 @@ class CourseScope(Scope): ) @classmethod - def get_or_create_for_external_key(cls, scope) -> "CourseScope": - """Get or create a CourseScope for the given external key. - - The backing CourseOverview need not exist yet (e.g. during a course - rerun, the destination course id is known before the course is - cloned): the scope is created with ``course_overview=None`` in that - case and gets linked up automatically once a CourseOverview with a - matching id is saved (see link_pending_scope() and the backfill - signal receiver in openedx_authz/handlers.py). - - Args: - scope: ScopeData object with an external_key attribute containing - a CourseKey string. - - Returns: - CourseScope: The Scope instance for the given CourseOverview, - or None if the scope is a glob pattern (contains wildcard). - """ - course_overview = scope.get_object() - if course_overview is None: - # The course doesn't exist yet: key the row by its external_key so it can be - # found and linked up later by link_pending_scope(). - row, _ = cls.objects.get_or_create(external_key=scope.external_key) - return row - - # Look up by the FK first (as before this change) so scopes created before the - # external_key field existed are reused rather than duplicated. - row, created = cls.objects.get_or_create( - course_overview=course_overview, - defaults={"external_key": scope.external_key}, - ) - if not created and not row.external_key: - row.external_key = scope.external_key - row.save(update_fields=["external_key"]) - return row - - @classmethod - def link_pending_scope(cls, course_overview) -> None: - """Link a pending CourseScope to the CourseOverview that was just created for it. - - Called from the CourseOverview post_save signal receiver in openedx_authz/handlers.py - once a course with a matching external_key shows up. - - Args: - course_overview: The CourseOverview instance that was just saved. - """ - cls.objects.filter( - external_key=str(course_overview.id), course_overview__isnull=True - ).update(course_overview=course_overview) + def external_key_for_object(cls, linked_object) -> str: + """Return the canonical external_key string for a CourseOverview instance.""" + return str(linked_object.id)