refactor: consider when deleting extended model so the casbin rule is also deleted

Author: mariajgrimaldi

openedx_authz/apps.py

@@ -13,6 +13,7 @@ class OpenedxAuthzConfig(AppConfig):
     name = "openedx_authz"
     verbose_name = "Open edX AuthZ"
     default_auto_field = "django.db.models.BigAutoField"
+
     plugin_app = {
         "url_config": {
             "lms.djangoapp": {
@@ -39,3 +40,7 @@ class OpenedxAuthzConfig(AppConfig):
             },
         },
     }
+
+    def ready(self):
+        """Import signal handlers when Django starts."""
+        import openedx_authz.handlers

openedx_authz/handlers.py

@@ -0,0 +1,45 @@
+"""Signal handlers for the authorization framework.
+
+These handlers ensure proper cleanup and consistency when models are deleted.
+"""
+
+from casbin_adapter.models import CasbinRule
+from django.db.models.signals import post_delete
+from django.dispatch import receiver
+
+from openedx_authz.models.core import ExtendedCasbinRule
+
+import logging
+
+logger = logging.getLogger(__name__)
+
+
+@receiver(post_delete, sender=ExtendedCasbinRule)
+def delete_casbin_rule_on_extended_rule_deletion(sender, instance, **kwargs):
+    """Delete the companion CasbinRule after its ExtendedCasbinRule disappears.
+
+    The handler keeps authorization data symmetric with three common flows:
+    - Direct ExtendedCasbinRule deletes (API/UI) trigger removal of the linked CasbinRule.
+    - Cascades from `Scope` or `Subject` deletions clear their ExtendedCasbinRule rows and, via this handler, the matching CasbinRule entries.
+    - Cascades initiated from the CasbinRule side (enforcer cleanups) leave the query as a no-op because the row is already gone.
+
+    Running on ``post_delete`` ensures database cascades complete before the cleanup runs, so
+    enforcer-driven deletions no longer raise false errors.
+
+    Args:
+        sender: The model class (ExtendedCasbinRule).
+        instance: The ExtendedCasbinRule instance being deleted.
+        **kwargs: Additional keyword arguments from the signal.
+    """
+    try:
+        # Rely on delete() being idempotent; returns 0 rows if the CasbinRule was
+        # already removed (for example, because it triggered this signal).
+        CasbinRule.objects.filter(id=instance.casbin_rule_id).delete()
+    except Exception as exc:
+        # Log but don't raise - we don't want to break the deletion of
+        # ExtendedCasbinRule if something goes wrong while deleting the CasbinRule.
+        logger.exception(
+            "Error deleting CasbinRule %s during ExtendedCasbinRule cleanup",
+            instance.casbin_rule_id,
+            exc_info=exc,
+        )

openedx_authz/migrations/0001_initial.py

@@ -49,7 +49,7 @@ class Migration(migrations.Migration):
                 ('created_at', models.DateTimeField(auto_now_add=True)),
                 ('updated_at', models.DateTimeField(auto_now=True)),
                 ('metadata', models.JSONField(blank=True, null=True)),
-                ('casbin_rule', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='extended_rule', to='casbin_adapter.casbinrule')),
+                ('casbin_rule', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='extended_rule', to='casbin_adapter.casbinrule')),
                 ('scope', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='casbin_rules', to='openedx_authz.scope')),
                 ('subject', models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='casbin_rules', to='openedx_authz.subject')),
             ],
@@ -66,7 +66,6 @@ class Migration(migrations.Migration):
                     'content_library',
                     models.ForeignKey(
                         blank=True,
-                        db_constraint=False,
                         null=True,
                         on_delete=django.db.models.deletion.CASCADE,
                         related_name='authz_scopes',

openedx_authz/migrations/0003_alter_extendedcasbinrule_casbin_rule.py

@@ -0,0 +1,20 @@
+# Generated by Django 5.2.7 on 2025-10-21 16:41
+
+import django.db.models.deletion
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('casbin_adapter', '0001_initial'),
+        ('openedx_authz', '0002_alter_contentlibraryscope_scope_ptr'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='extendedcasbinrule',
+            name='casbin_rule',
+            field=models.OneToOneField(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='extended_rule', to='casbin_adapter.casbinrule'),
+        ),
+    ]

openedx_authz/models/core.py

@@ -126,12 +126,14 @@ class ExtendedCasbinRule(models.Model):
     package to include additional fields for storing metadata about each rule.
     """
 
-    # Instead of making it 1:1 only with the CasbinRule primary key which we usually don't know, let's
-    # make an unique key based on the casbin_rule field which is a concatenation of all the fields
-    # in the CasbinRule model. This way, we can easily look up the ExtendedCasbinRule
-    # based on a policy line which SHOULD be unique.
+    # OneToOne relationship ensures each CasbinRule has at most one ExtendedCasbinRule.
+    # We also maintain a unique key based on the casbin_rule field components for easy lookup
+    # based on a policy line (ptype,v0,v1,v2,v3) which should be unique.
+    #
+    # Note: We use CASCADE here. When CasbinRule is deleted, ExtendedCasbinRule is also deleted.
+    # The signal handler in handlers.py ensures the reverse (ExtendedCasbinRule deletion → CasbinRule deletion).
     casbin_rule_key = models.CharField(max_length=255, unique=True)
-    casbin_rule = models.ForeignKey(
+    casbin_rule = models.OneToOneField(
         "casbin_adapter.CasbinRule",
         on_delete=models.CASCADE,
         related_name="extended_rule",

openedx_authz/models/scopes.py

@@ -46,11 +46,6 @@ class ContentLibraryScope(Scope):
     # Piggybacking on the existing ContentLibrary model to keep the ExtendedCasbinRule up to date
     # by deleting the Scope, and thus the ExtendedCasbinRule, when the ContentLibrary is deleted.
     #
-    # Using string reference with db_constraint=False allows this model to work even when
-    # the content_libraries app is not installed. The ForeignKey relationship is maintained
-    # at the application level, but Django won't enforce referential integrity in the database
-    # or validate that the app exists during model loading.
-    #
     # When content_libraries IS available, the on_delete=CASCADE will still work at the
     # application level through Django's signal handlers.
     # Use a string reference to the external app's model so Django won't try
@@ -66,7 +61,6 @@ class ContentLibraryScope(Scope):
         null=True,
         blank=True,
         related_name="authz_scopes",
-        db_constraint=False,  # Don't enforce FK constraint - allows working without content_libraries app
     )
 
     @classmethod

openedx_authz/settings/test.py

@@ -37,7 +37,6 @@ def plugin_settings(settings):  # pylint: disable=unused-argument
     "django.contrib.sessions",
     "openedx_authz.engine.apps.CasbinAdapterConfig",
     "openedx_authz.apps.OpenedxAuthzConfig",
-    "casbin_adapter.apps.CasbinAdapterConfig",
     "openedx_authz.tests.stubs.apps.StubsConfig",
 )