diff --git a/CHANGELOG.rst b/CHANGELOG.rst index f119f409..12fe351a 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -16,6 +16,15 @@ Unreleased * +0.19.0 - 2025-11-18 +******************** + +Added +===== + +* Handle cache invalidation via a uuid in the database to ensure policy reloads + occur only when necessary. + 0.18.0 - 2025-11-17 ******************** diff --git a/openedx_authz/__init__.py b/openedx_authz/__init__.py index 01796def..1750d297 100644 --- a/openedx_authz/__init__.py +++ b/openedx_authz/__init__.py @@ -4,6 +4,6 @@ import os -__version__ = "0.18.0" +__version__ = "0.19.0" ROOT_DIRECTORY = os.path.dirname(os.path.abspath(__file__)) diff --git a/openedx_authz/api/roles.py b/openedx_authz/api/roles.py index e5da5abc..c5b8cb78 100644 --- a/openedx_authz/api/roles.py +++ b/openedx_authz/api/roles.py @@ -219,6 +219,9 @@ def assign_role_to_subject_in_scope(subject: SubjectData, role: RoleData, scope: ) if not extended_rule: raise Exception("Failed to create ExtendedCasbinRule for the assignment") + + # Invalidate policy cache to ensure changes are picked up + AuthzEnforcer.invalidate_policy_cache() return True @@ -245,7 +248,12 @@ def unassign_role_from_subject_in_scope(subject: SubjectData, role: RoleData, sc bool: True if the role was unassigned successfully, False otherwise. """ enforcer = AuthzEnforcer.get_enforcer() - return enforcer.delete_roles_for_user_in_domain(subject.namespaced_key, role.namespaced_key, scope.namespaced_key) + success = enforcer.delete_roles_for_user_in_domain( + subject.namespaced_key, role.namespaced_key, scope.namespaced_key + ) + # Invalidate policy cache to ensure changes are picked up + AuthzEnforcer.invalidate_policy_cache() + return success def batch_unassign_role_from_subjects_in_scope(subjects: list[SubjectData], role: RoleData, scope: ScopeData) -> None: diff --git a/openedx_authz/engine/enforcer.py b/openedx_authz/engine/enforcer.py index cc76d1cb..a0caad9a 100644 --- a/openedx_authz/engine/enforcer.py +++ b/openedx_authz/engine/enforcer.py @@ -16,12 +16,14 @@ """ import logging +from uuid import uuid4 from casbin import SyncedEnforcer from casbin_adapter.enforcer import initialize_enforcer from django.conf import settings from openedx_authz.engine.adapter import ExtendedAdapter +from openedx_authz.models.engine import PolicyCacheControl def libraries_v2_enabled() -> bool: @@ -68,6 +70,7 @@ class AuthzEnforcer: _enforcer = None _adapter = None + _last_policy_loaded_version = None def __new__(cls): """Singleton pattern to ensure a single enforcer instance.""" @@ -153,6 +156,45 @@ def configure_enforcer_auto_save_and_load(cls): cls.configure_enforcer_auto_save(auto_save_policy) + @classmethod + def load_policy_if_needed(cls): + """Load policy if the last load version indicates it's needed. + + This method checks if the policy needs to be reloaded comparing + the last load version with the version in the cache invalidation model, + and reloads it if necessary. + + Returns: + None + """ + last_version = PolicyCacheControl.get_version() + + if last_version is None: + # No version in cache control; initialize it + last_version = uuid4() + PolicyCacheControl.set_version(last_version) + logger.info("Initialized policy last modified version in cache control.") + + if cls._last_policy_loaded_version is None or last_version != cls._last_policy_loaded_version: + # Policy has been modified since last load; reload it + cls._enforcer.load_policy() + cls._last_policy_loaded_version = last_version + logger.info(f"Reloaded policy to version {last_version}") + + @classmethod + def invalidate_policy_cache(cls): + """Invalidate the current policy cache to force a reload on next check. + + This method updates the last modified version in the cache invalidation model + to a new UUID, indicating that the policy has changed. + + Returns: + None + """ + new_version = uuid4() + PolicyCacheControl.set_version(new_version) + logger.info(f"Invalidated policy cache to version {new_version}") + @classmethod def get_enforcer(cls) -> SyncedEnforcer: """Get the enforcer instance, creating it if needed. @@ -163,6 +205,9 @@ def get_enforcer(cls) -> SyncedEnforcer: if cls._enforcer is None: cls._enforcer = cls._initialize_enforcer() + # (re)load policy if needed + cls.load_policy_if_needed() + # HACK: This code block will only be useful when in Ulmo to deactivate # the enforcer when the new library experience is disabled. It should be # removed for the next release cycle. diff --git a/openedx_authz/migrations/0005_policycachecontrol.py b/openedx_authz/migrations/0005_policycachecontrol.py new file mode 100644 index 00000000..98ccf115 --- /dev/null +++ b/openedx_authz/migrations/0005_policycachecontrol.py @@ -0,0 +1,21 @@ +# Generated by Django 4.2.24 on 2025-11-14 22:38 + +import uuid + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("openedx_authz", "0004_contentlibraryscope"), + ] + + operations = [ + migrations.CreateModel( + name="PolicyCacheControl", + fields=[ + ("id", models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), + ("version", models.UUIDField(default=uuid.uuid4)), + ], + ), + ] diff --git a/openedx_authz/migrations/0005_migrate_legacy_permissions.py b/openedx_authz/migrations/0006_migrate_legacy_permissions.py similarity index 97% rename from openedx_authz/migrations/0005_migrate_legacy_permissions.py rename to openedx_authz/migrations/0006_migrate_legacy_permissions.py index 95a7f97d..4372c8fe 100644 --- a/openedx_authz/migrations/0005_migrate_legacy_permissions.py +++ b/openedx_authz/migrations/0006_migrate_legacy_permissions.py @@ -53,7 +53,7 @@ class Migration(migrations.Migration): """ dependencies = [ - ("openedx_authz", "0004_contentlibraryscope"), + ("openedx_authz", "0005_policycachecontrol"), ] operations = [ diff --git a/openedx_authz/models/engine.py b/openedx_authz/models/engine.py new file mode 100644 index 00000000..cc6f8bc4 --- /dev/null +++ b/openedx_authz/models/engine.py @@ -0,0 +1,51 @@ +"""Models for the authorization engine.""" + +from uuid import UUID, uuid4 + +from django.db import models + + +class PolicyCacheControl(models.Model): + """Model to control policy cache invalidation. + + This model can be used to trigger cache invalidation for authorization policies + by changing the version. Whenever this model is updated, the authorization + engine should invalidate its cached policies. + + .. no_pii: + """ + + version = models.UUIDField(default=uuid4) + + def save(self, *args, **kwargs): + """Override save to ensure a single instance.""" + self.pk = 1 # Ensure a single instance + super().save(*args, **kwargs) + + @classmethod + def get(cls): + """Get the singleton instance of the model.""" + obj, _ = cls.objects.get_or_create(pk=1) + return obj + + @classmethod + def get_version(cls): + """Get the version for policy cache control. + + Returns: + UUID: The version of the last update. + """ + instance = cls.get() + return instance.version + + @classmethod + def set_version(cls, version: UUID): + """Update the cache version. + + This method updates the cache version, which can be used to signal + that the policy cache should be invalidated. + """ + instance = cls.get() + instance.version = version + + instance.save() diff --git a/openedx_authz/rest_api/v1/permissions.py b/openedx_authz/rest_api/v1/permissions.py index f681294d..d867b84f 100644 --- a/openedx_authz/rest_api/v1/permissions.py +++ b/openedx_authz/rest_api/v1/permissions.py @@ -5,7 +5,6 @@ from rest_framework.permissions import BasePermission from openedx_authz import api -from openedx_authz.engine.enforcer import AuthzEnforcer class PermissionMeta(type(BasePermission)): @@ -183,7 +182,6 @@ def has_permission(self, request, view) -> bool: """ if request.user.is_superuser or request.user.is_staff: return True - AuthzEnforcer.get_enforcer().load_policy() return self._get_permission_instance(request).has_permission(request, view) def has_object_permission(self, request, view, obj) -> bool: @@ -200,7 +198,6 @@ def has_object_permission(self, request, view, obj) -> bool: """ if request.user.is_superuser or request.user.is_staff: return True - AuthzEnforcer.get_enforcer().load_policy() return self._get_permission_instance(request).has_object_permission(request, view, obj) diff --git a/openedx_authz/rest_api/v1/views.py b/openedx_authz/rest_api/v1/views.py index a95d73fd..e560bc08 100644 --- a/openedx_authz/rest_api/v1/views.py +++ b/openedx_authz/rest_api/v1/views.py @@ -16,7 +16,6 @@ from openedx_authz import api from openedx_authz.constants import permissions -from openedx_authz.engine.enforcer import AuthzEnforcer from openedx_authz.rest_api.data import RoleOperationError, RoleOperationStatus from openedx_authz.rest_api.decorators import authz_permissions, view_auth_classes from openedx_authz.rest_api.utils import ( @@ -103,7 +102,6 @@ class PermissionValidationMeView(APIView): ) def post(self, request: HttpRequest) -> Response: """Validate one or more permissions for the authenticated user.""" - AuthzEnforcer.get_enforcer().load_policy() serializer = PermissionValidationSerializer(data=request.data, many=True) serializer.is_valid(raise_exception=True) diff --git a/openedx_authz/settings/common.py b/openedx_authz/settings/common.py index d2b55922..32dc2384 100644 --- a/openedx_authz/settings/common.py +++ b/openedx_authz/settings/common.py @@ -31,8 +31,10 @@ def plugin_settings(settings): # Set default CASBIN_AUTO_LOAD_POLICY_INTERVAL if not already set. # This setting defines how often (in seconds) the Casbin enforcer should # automatically reload policies from the database. + # By default, we set it to 0, which disables the auto-reload. + # As it shouldn't be needed thanks to cache invalidation. if not hasattr(settings, "CASBIN_AUTO_LOAD_POLICY_INTERVAL"): - settings.CASBIN_AUTO_LOAD_POLICY_INTERVAL = 5 + settings.CASBIN_AUTO_LOAD_POLICY_INTERVAL = 0 # Set default CASBIN_AUTO_SAVE_POLICY if not already set. # This setting defines whether the Casbin enforcer should automatically diff --git a/openedx_authz/tests/api/test_roles.py b/openedx_authz/tests/api/test_roles.py index 83163c51..197fe425 100644 --- a/openedx_authz/tests/api/test_roles.py +++ b/openedx_authz/tests/api/test_roles.py @@ -978,7 +978,6 @@ def test_assign_role_creates_extended_casbin_rule(self): self.assertIn(subject_data.namespaced_key, extended_rule.casbin_rule_key) self.assertIn(scope_data.namespaced_key, extended_rule.casbin_rule_key) - @ddt_data( # Test user with single role in single scope ("alice", ["lib:Org1:math_101"], {"library_admin"}), diff --git a/openedx_authz/tests/test_enforcer.py b/openedx_authz/tests/test_enforcer.py index 80bd75e2..ea12d855 100644 --- a/openedx_authz/tests/test_enforcer.py +++ b/openedx_authz/tests/test_enforcer.py @@ -7,6 +7,7 @@ import time from unittest.mock import patch +from uuid import uuid4 import casbin from ddt import data as ddt_data @@ -17,6 +18,7 @@ from openedx_authz.engine.enforcer import AuthzEnforcer from openedx_authz.engine.filter import Filter from openedx_authz.engine.utils import migrate_policy_between_enforcers +from openedx_authz.models.engine import PolicyCacheControl from openedx_authz.tests.test_utils import make_action_key, make_role_key, make_scope_key, make_user_key @@ -823,3 +825,108 @@ def test_multiple_get_enforcer_calls_preserve_auto_save(self, mock_toggle): for _ in range(5): AuthzEnforcer.get_enforcer() self.assertTrue(AuthzEnforcer.is_auto_save_enabled()) + + +class TestEnforcerPolicyCacheBehavior(TransactionTestCase): + """Test cases for enforcer policy cache behavior. + + These tests verify that the policy cache logic works correctly, + ensuring that policies are reloaded only when needed. + """ + + @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") + @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0) + def test_load_policy_if_needed_initializes_cache_version(self, mock_toggle): + """Test that load_policy_if_needed initializes cache version on first call. + + Expected result: + - On first call, cache invalidation model is initialized + - Policy is loaded since last load version is None + """ + mock_toggle.return_value = True + + AuthzEnforcer._last_policy_loaded_version = None # pylint: disable=protected-access + # get_enforcer calls load_policy_if_needed internally + AuthzEnforcer.get_enforcer() + + cached_version = PolicyCacheControl.get_version() + self.assertIsNotNone(cached_version) + self.assertIsNotNone(AuthzEnforcer._last_policy_loaded_version) # pylint: disable=protected-access + self.assertEqual( + AuthzEnforcer._last_policy_loaded_version, # pylint: disable=protected-access + cached_version, + ) + + @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") + @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0) + def test_load_policy_if_needed_loads_when_stale(self, mock_toggle): + """Test that load_policy_if_needed reloads policy when stale. + + Expected result: + - If policy is stale, it is reloaded + - _last_policy_loaded_version is updated with new version + """ + mock_toggle.return_value = True + + stale_version = uuid4() + current_version = uuid4() + + # Set last loaded version to stale value + AuthzEnforcer._last_policy_loaded_version = stale_version # pylint: disable=protected-access + # Set last cache invalidation current version + PolicyCacheControl.set_version(current_version) + + # get_enforcer calls load_policy_if_needed internally + AuthzEnforcer.get_enforcer() + + self.assertIsNotNone(AuthzEnforcer._last_policy_loaded_version) # pylint: disable=protected-access + self.assertEqual( + AuthzEnforcer._last_policy_loaded_version, # pylint: disable=protected-access + current_version, + ) + + @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") + @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0) + def test_load_policy_if_needed_doesnt_reload_when_not_stale(self, mock_toggle): + """Test that load_policy_if_needed does not reload policy when not stale. + + Expected result: + - If policy is not stale, it is not reloaded + - _last_policy_loaded_version remains unchanged + """ + mock_toggle.return_value = True + + current_version = uuid4() + + # Set last loaded version to current version + AuthzEnforcer._last_policy_loaded_version = current_version # pylint: disable=protected-access + # Set last cache invalidation to same version + PolicyCacheControl.set_version(current_version) + + # get_enforcer calls load_policy_if_needed internally + AuthzEnforcer.get_enforcer() + + self.assertEqual( + AuthzEnforcer._last_policy_loaded_version, # pylint: disable=protected-access + current_version, + ) + + @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") + @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0) + def test_invalidate_policy_cache(self, mock_toggle): + """Test that invalidate_policy_cache updates the cache invalidation model. + + Expected result: + - Cache invalidation key is updated to a new version + """ + mock_toggle.return_value = True + + AuthzEnforcer._last_policy_loaded_version = uuid4() # pylint: disable=protected-access + old_cache_value = uuid4() + PolicyCacheControl.set_version(old_cache_value) + + AuthzEnforcer.invalidate_policy_cache() + + new_cache_value = PolicyCacheControl.get_version() + self.assertIsNotNone(new_cache_value) + self.assertNotEqual(new_cache_value, old_cache_value) diff --git a/openedx_authz/tests/test_imports.py b/openedx_authz/tests/test_imports.py index e22d79df..6efedb19 100644 --- a/openedx_authz/tests/test_imports.py +++ b/openedx_authz/tests/test_imports.py @@ -1,4 +1,5 @@ """Test module imports.""" + import sys from unittest import TestCase @@ -7,20 +8,19 @@ class TestImports(TestCase): """Test that imports work correctly.""" def setUp(self): - """Remove cached modules to ensure fresh imports and detect circular dependencies. - """ + """Remove cached modules to ensure fresh imports and detect circular dependencies.""" super().setUp() # List of modules to remove from cache to test fresh imports modules_to_clear = [ - 'openedx_authz.engine.enforcer', - 'openedx_authz.engine.matcher', - 'openedx_authz.engine.adapter', - 'openedx_authz.api', - 'openedx_authz.api.permissions', - 'openedx_authz.api.roles', - 'openedx_authz.api.users', - 'openedx_authz.api.data', + "openedx_authz.engine.enforcer", + "openedx_authz.engine.matcher", + "openedx_authz.engine.adapter", + "openedx_authz.api", + "openedx_authz.api.permissions", + "openedx_authz.api.roles", + "openedx_authz.api.users", + "openedx_authz.api.data", ] for module_name in modules_to_clear: @@ -30,6 +30,7 @@ def setUp(self): def test_import_authzenforcer(self): """Test that AuthzEnforcer can be imported.""" from openedx_authz.engine.enforcer import AuthzEnforcer # pylint: disable=import-outside-toplevel + try: self.assertIsNotNone(AuthzEnforcer) except ImportError as e: diff --git a/openedx_authz/tests/test_models.py b/openedx_authz/tests/test_models.py index 4bdf77ca..d8a15f9f 100644 --- a/openedx_authz/tests/test_models.py +++ b/openedx_authz/tests/test_models.py @@ -13,6 +13,8 @@ which run against the real ContentLibrary model. """ +from uuid import UUID, uuid4 + from casbin_adapter.models import CasbinRule from django.contrib.auth import get_user_model from django.test import TestCase @@ -20,6 +22,7 @@ from openedx_authz.api.data import ContentLibraryData, UserData from openedx_authz.models import ExtendedCasbinRule, Scope, Subject +from openedx_authz.models.engine import PolicyCacheControl from openedx_authz.tests.stubs.models import ContentLibrary User = get_user_model() @@ -136,3 +139,39 @@ def test_extended_casbin_rule_cascade_deletion_when_subject_deleted(self): self.assertFalse(ExtendedCasbinRule.objects.filter(id=extended_rule_id).exists()) self.assertFalse(CasbinRule.objects.filter(id=casbin_rule_id).exists()) self.assertFalse(Subject.objects.filter(id=subject_id).exists()) + + +class TestPolicyCacheControlModel(TestCase): + """Test cases for the PolicyCacheControl model.""" + + def test_get_and_set_version(self): + """Test getting and setting the cache version. + + Expected Result: + - Initially, the version is set to a UUID. + - After setting a new version, it reflects the updated value. + """ + initial_version = PolicyCacheControl.get_version() + self.assertIsInstance(initial_version, UUID) + + new_version = uuid4() # Generate a new UUID + PolicyCacheControl.set_version(new_version) + + updated_version = PolicyCacheControl.get_version() + self.assertEqual(updated_version, new_version) + + def test_singleton_behavior(self): + """Test that only one instance of PolicyCacheControl exists. + + Expected Result: + - Multiple calls to get() return the same instance. + - Saving the instance does not create duplicates. + """ + instance1 = PolicyCacheControl.get() + instance2 = PolicyCacheControl.get() + + self.assertEqual(instance1.id, instance2.id) + + instance1.save() + all_instances = PolicyCacheControl.objects.all() + self.assertEqual(all_instances.count(), 1)