From a6558270409619b994e2233702bf27a42cf06eb6 Mon Sep 17 00:00:00 2001 From: Rodrigo Mendez Date: Tue, 11 Nov 2025 16:46:53 -0600 Subject: [PATCH 01/12] feat: Experimenting with cache invalidation --- openedx_authz/api/roles.py | 8 +++- openedx_authz/engine/enforcer.py | 60 ++++++++++++++++++++++-- openedx_authz/rest_api/v1/permissions.py | 4 +- openedx_authz/rest_api/v1/views.py | 2 +- 4 files changed, 66 insertions(+), 8 deletions(-) diff --git a/openedx_authz/api/roles.py b/openedx_authz/api/roles.py index e5da5abc..656c3a06 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,10 @@ 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..6b895621 100644 --- a/openedx_authz/engine/enforcer.py +++ b/openedx_authz/engine/enforcer.py @@ -16,10 +16,12 @@ """ import logging +import time from casbin import SyncedEnforcer from casbin_adapter.enforcer import initialize_enforcer from django.conf import settings +from django.core.cache import cache from openedx_authz.engine.adapter import ExtendedAdapter @@ -66,8 +68,11 @@ class AuthzEnforcer: _adapter (ExtendedAdapter): The singleton adapter instance. """ + CACHE_KEY = "authz_policy_last_modified_timestamp" + _enforcer = None _adapter = None + _last_policy_load_timestamp = None def __new__(cls): """Singleton pattern to ensure a single enforcer instance.""" @@ -146,13 +151,57 @@ def configure_enforcer_auto_save_and_load(cls): auto_load_policy_interval = getattr(settings, "CASBIN_AUTO_LOAD_POLICY_INTERVAL", 0) auto_save_policy = getattr(settings, "CASBIN_AUTO_SAVE_POLICY", True) - if auto_load_policy_interval > 0: - cls.configure_enforcer_auto_loading(auto_load_policy_interval) - else: - logger.warning("CASBIN_AUTO_LOAD_POLICY_INTERVAL is not set or zero; auto-load is disabled.") + # TODO: remove autoload in favor of cache invalidation? + # if auto_load_policy_interval > 0: + # cls.configure_enforcer_auto_loading(auto_load_policy_interval) + # else: + # logger.warning("CASBIN_AUTO_LOAD_POLICY_INTERVAL is not set or zero; auto-load is disabled.") cls.configure_enforcer_auto_save(auto_save_policy) + @classmethod + def load_policy_if_needed(cls): + """Load policy if the last load timestamp indicates it's needed. + + This method checks if the policy needs to be reloaded comparing + the last load timestamp with the last modified timestamp in cache + and reloads it if necessary. + + Returns: + None + """ + last_modified_timestamp = cache.get(cls.CACHE_KEY) + + current_timestamp = time.time() + + if last_modified_timestamp is None: + # No timestamp in cache; initialize it + cache.set(cls.CACHE_KEY, current_timestamp, None) + logger.info(f">>>> Initialized policy last modified timestamp in cache. {current_timestamp}") + + if ( + cls._last_policy_load_timestamp is None or + last_modified_timestamp > cls._last_policy_load_timestamp + ): + # Policy has been modified since last load; reload it + cls._enforcer.load_policy() + cls._last_policy_load_timestamp = current_timestamp + logger.info(f">>>> Reloaded policy at {current_timestamp}") + + @classmethod + def invalidate_policy_cache(cls): + """Invalidate the current policy cache to force a reload on next check. + + This method updates the last modified timestamp in the cache to + the current time, indicating that the policy has changed. + + Returns: + None + """ + current_timestamp = time.time() + cache.set(cls.CACHE_KEY, current_timestamp, None) + logger.info(f">>>> Invalidated policy cache at {current_timestamp}") + @classmethod def get_enforcer(cls) -> SyncedEnforcer: """Get the enforcer instance, creating it if needed. @@ -163,6 +212,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/rest_api/v1/permissions.py b/openedx_authz/rest_api/v1/permissions.py index f681294d..e2efb54e 100644 --- a/openedx_authz/rest_api/v1/permissions.py +++ b/openedx_authz/rest_api/v1/permissions.py @@ -183,7 +183,7 @@ def has_permission(self, request, view) -> bool: """ if request.user.is_superuser or request.user.is_staff: return True - AuthzEnforcer.get_enforcer().load_policy() + # 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 +200,7 @@ 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() + # 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..511ea409 100644 --- a/openedx_authz/rest_api/v1/views.py +++ b/openedx_authz/rest_api/v1/views.py @@ -103,7 +103,7 @@ class PermissionValidationMeView(APIView): ) def post(self, request: HttpRequest) -> Response: """Validate one or more permissions for the authenticated user.""" - AuthzEnforcer.get_enforcer().load_policy() + # AuthzEnforcer.get_enforcer().load_policy() serializer = PermissionValidationSerializer(data=request.data, many=True) serializer.is_valid(raise_exception=True) From e35079e65eb128530c1c309b650b0a289a8ebc46 Mon Sep 17 00:00:00 2001 From: Rodrigo Mendez Date: Wed, 12 Nov 2025 09:36:25 -0600 Subject: [PATCH 02/12] squash!: Cleanup --- openedx_authz/engine/enforcer.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/openedx_authz/engine/enforcer.py b/openedx_authz/engine/enforcer.py index 6b895621..030f9931 100644 --- a/openedx_authz/engine/enforcer.py +++ b/openedx_authz/engine/enforcer.py @@ -148,7 +148,8 @@ def configure_enforcer_auto_save_and_load(cls): Returns: None """ - auto_load_policy_interval = getattr(settings, "CASBIN_AUTO_LOAD_POLICY_INTERVAL", 0) + # auto_load_policy_interval = getattr( + # settings, "CASBIN_AUTO_LOAD_POLICY_INTERVAL", 0) auto_save_policy = getattr(settings, "CASBIN_AUTO_SAVE_POLICY", True) # TODO: remove autoload in favor of cache invalidation? @@ -177,7 +178,8 @@ def load_policy_if_needed(cls): if last_modified_timestamp is None: # No timestamp in cache; initialize it cache.set(cls.CACHE_KEY, current_timestamp, None) - logger.info(f">>>> Initialized policy last modified timestamp in cache. {current_timestamp}") + logger.info( + f">>>> Initialized policy last modified timestamp in cache. {current_timestamp}") if ( cls._last_policy_load_timestamp is None or @@ -263,11 +265,13 @@ def _initialize_enforcer(cls) -> SyncedEnforcer: # issues when the app is not fully loaded (e.g., while pulling translations, etc.). initialize_enforcer(db_alias) except Exception as e: - logger.error(f"Failed to initialize Casbin enforcer with DB alias '{db_alias}': {e}") + logger.error( + f"Failed to initialize Casbin enforcer with DB alias '{db_alias}': {e}") raise adapter = ExtendedAdapter() enforcer = SyncedEnforcer(settings.CASBIN_MODEL, adapter) - enforcer.add_function("is_staff_or_superuser", is_admin_or_superuser_check) + enforcer.add_function("is_staff_or_superuser", + is_admin_or_superuser_check) return enforcer From fc902916407f933e723b47f5c65b3711138f6bce Mon Sep 17 00:00:00 2001 From: Rodrigo Mendez Date: Wed, 12 Nov 2025 13:20:24 -0600 Subject: [PATCH 03/12] squash!: Re-enable autoload setting, but default to disabled --- openedx_authz/engine/enforcer.py | 12 ++++++------ openedx_authz/settings/common.py | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/openedx_authz/engine/enforcer.py b/openedx_authz/engine/enforcer.py index 030f9931..8d831071 100644 --- a/openedx_authz/engine/enforcer.py +++ b/openedx_authz/engine/enforcer.py @@ -148,15 +148,15 @@ def configure_enforcer_auto_save_and_load(cls): Returns: None """ - # auto_load_policy_interval = getattr( - # settings, "CASBIN_AUTO_LOAD_POLICY_INTERVAL", 0) + auto_load_policy_interval = getattr( + settings, "CASBIN_AUTO_LOAD_POLICY_INTERVAL", 0) auto_save_policy = getattr(settings, "CASBIN_AUTO_SAVE_POLICY", True) # TODO: remove autoload in favor of cache invalidation? - # if auto_load_policy_interval > 0: - # cls.configure_enforcer_auto_loading(auto_load_policy_interval) - # else: - # logger.warning("CASBIN_AUTO_LOAD_POLICY_INTERVAL is not set or zero; auto-load is disabled.") + if auto_load_policy_interval > 0: + cls.configure_enforcer_auto_loading(auto_load_policy_interval) + else: + logger.warning("CASBIN_AUTO_LOAD_POLICY_INTERVAL is not set or zero; auto-load is disabled.") cls.configure_enforcer_auto_save(auto_save_policy) diff --git a/openedx_authz/settings/common.py b/openedx_authz/settings/common.py index d2b55922..d737736a 100644 --- a/openedx_authz/settings/common.py +++ b/openedx_authz/settings/common.py @@ -32,7 +32,7 @@ def plugin_settings(settings): # This setting defines how often (in seconds) the Casbin enforcer should # automatically reload policies from the database. 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 From bccdd3d38ba8fbaf4357da2b42aade372510395a Mon Sep 17 00:00:00 2001 From: Rodrigo Mendez Date: Wed, 12 Nov 2025 14:48:42 -0600 Subject: [PATCH 04/12] squash!: Cleanup --- openedx_authz/api/roles.py | 6 ++++-- openedx_authz/rest_api/v1/permissions.py | 3 --- openedx_authz/rest_api/v1/views.py | 2 -- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/openedx_authz/api/roles.py b/openedx_authz/api/roles.py index 656c3a06..c5b8cb78 100644 --- a/openedx_authz/api/roles.py +++ b/openedx_authz/api/roles.py @@ -219,7 +219,7 @@ 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 @@ -248,7 +248,9 @@ 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() - success = 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 diff --git a/openedx_authz/rest_api/v1/permissions.py b/openedx_authz/rest_api/v1/permissions.py index e2efb54e..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 511ea409..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) From 132b339af7279e2872b1a3c56ae2834a4e208ca0 Mon Sep 17 00:00:00 2001 From: Rodrigo Mendez Date: Wed, 12 Nov 2025 16:25:51 -0600 Subject: [PATCH 05/12] squash!: Add tests --- openedx_authz/tests/test_enforcer.py | 103 +++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/openedx_authz/tests/test_enforcer.py b/openedx_authz/tests/test_enforcer.py index 80bd75e2..ae186e99 100644 --- a/openedx_authz/tests/test_enforcer.py +++ b/openedx_authz/tests/test_enforcer.py @@ -12,6 +12,7 @@ from ddt import data as ddt_data from ddt import ddt from django.conf import settings +from django.core.cache import cache from django.test import TestCase, TransactionTestCase, override_settings from openedx_authz.engine.enforcer import AuthzEnforcer @@ -823,3 +824,105 @@ 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_timestamp(self, mock_toggle): + """Test that load_policy_if_needed initializes cache timestamp on first call. + + Expected result: + - On first call, cache invalidation key is set with current timestamp + - Policy is loaded since last load timestamp is None + """ + mock_toggle.return_value = True + + AuthzEnforcer._last_policy_load_timestamp = None # pylint: disable=protected-access + cache.clear() + + # get_enforcer calls load_policy_if_needed internally + AuthzEnforcer.get_enforcer() + + cached_timestamp = cache.get(AuthzEnforcer.CACHE_KEY) + self.assertIsNotNone(cached_timestamp) + self.assertIsNotNone(AuthzEnforcer._last_policy_load_timestamp) # pylint: disable=protected-access + + @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_load_timestamp is updated with new timestamp + """ + mock_toggle.return_value = True + + now = time.time() + stale_timestamp = now - 60 # 60 seconds ago + + # Set last load timestamp to stale value + AuthzEnforcer._last_policy_load_timestamp = stale_timestamp # pylint: disable=protected-access + # Set last cache invalidation to a more recent time + cache.set(AuthzEnforcer.CACHE_KEY, now, None) + + # get_enforcer calls load_policy_if_needed internally + AuthzEnforcer.get_enforcer() + + self.assertIsNotNone(AuthzEnforcer._last_policy_load_timestamp) # pylint: disable=protected-access + self.assertGreater( + AuthzEnforcer._last_policy_load_timestamp, # pylint: disable=protected-access + stale_timestamp, + ) + + @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_load_timestamp remains unchanged + """ + mock_toggle.return_value = True + + now = time.time() + + # Set last load timestamp to current time + AuthzEnforcer._last_policy_load_timestamp = now # pylint: disable=protected-access + # Set last cache invalidation to an earlier time + cache.set(AuthzEnforcer.CACHE_KEY, now - 60, None) # 60 seconds ago + + # get_enforcer calls load_policy_if_needed internally + AuthzEnforcer.get_enforcer() + + self.assertEqual( + AuthzEnforcer._last_policy_load_timestamp, # pylint: disable=protected-access + now, + ) + + @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 key. + + Expected result: + - Cache invalidation key is updated with current timestamp + """ + mock_toggle.return_value = True + + AuthzEnforcer._last_policy_load_timestamp = time.time() # pylint: disable=protected-access + old_cache_value = time.time() - 60 # 60 seconds ago + cache.set(AuthzEnforcer.CACHE_KEY, old_cache_value, None) + + AuthzEnforcer.invalidate_policy_cache() + + new_cache_value = cache.get(AuthzEnforcer.CACHE_KEY) + self.assertIsNotNone(new_cache_value) + self.assertGreater(new_cache_value, old_cache_value) From a45426e87e3b9e3d5d69708930832d172472bc2e Mon Sep 17 00:00:00 2001 From: Rodrigo Mendez Date: Wed, 12 Nov 2025 16:59:47 -0600 Subject: [PATCH 06/12] squash!: Update Changelog and version --- CHANGELOG.rst | 9 +++++++++ openedx_authz/__init__.py | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) 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__)) From a4292b5a67892ba6202f431c2f71bdb910d42773 Mon Sep 17 00:00:00 2001 From: Rodrigo Mendez Date: Thu, 13 Nov 2025 09:31:23 -0600 Subject: [PATCH 07/12] squash!: Format code, bump version --- openedx_authz/engine/enforcer.py | 19 ++++++------------- openedx_authz/settings/common.py | 2 ++ openedx_authz/tests/test_enforcer.py | 1 + 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/openedx_authz/engine/enforcer.py b/openedx_authz/engine/enforcer.py index 8d831071..4c7c0702 100644 --- a/openedx_authz/engine/enforcer.py +++ b/openedx_authz/engine/enforcer.py @@ -148,8 +148,7 @@ def configure_enforcer_auto_save_and_load(cls): Returns: None """ - auto_load_policy_interval = getattr( - settings, "CASBIN_AUTO_LOAD_POLICY_INTERVAL", 0) + auto_load_policy_interval = getattr(settings, "CASBIN_AUTO_LOAD_POLICY_INTERVAL", 0) auto_save_policy = getattr(settings, "CASBIN_AUTO_SAVE_POLICY", True) # TODO: remove autoload in favor of cache invalidation? @@ -165,7 +164,7 @@ def load_policy_if_needed(cls): """Load policy if the last load timestamp indicates it's needed. This method checks if the policy needs to be reloaded comparing - the last load timestamp with the last modified timestamp in cache + the last load timestamp with the last modified timestamp in cache and reloads it if necessary. Returns: @@ -178,13 +177,9 @@ def load_policy_if_needed(cls): if last_modified_timestamp is None: # No timestamp in cache; initialize it cache.set(cls.CACHE_KEY, current_timestamp, None) - logger.info( - f">>>> Initialized policy last modified timestamp in cache. {current_timestamp}") + logger.info(f">>>> Initialized policy last modified timestamp in cache. {current_timestamp}") - if ( - cls._last_policy_load_timestamp is None or - last_modified_timestamp > cls._last_policy_load_timestamp - ): + if cls._last_policy_load_timestamp is None or last_modified_timestamp > cls._last_policy_load_timestamp: # Policy has been modified since last load; reload it cls._enforcer.load_policy() cls._last_policy_load_timestamp = current_timestamp @@ -265,13 +260,11 @@ def _initialize_enforcer(cls) -> SyncedEnforcer: # issues when the app is not fully loaded (e.g., while pulling translations, etc.). initialize_enforcer(db_alias) except Exception as e: - logger.error( - f"Failed to initialize Casbin enforcer with DB alias '{db_alias}': {e}") + logger.error(f"Failed to initialize Casbin enforcer with DB alias '{db_alias}': {e}") raise adapter = ExtendedAdapter() enforcer = SyncedEnforcer(settings.CASBIN_MODEL, adapter) - enforcer.add_function("is_staff_or_superuser", - is_admin_or_superuser_check) + enforcer.add_function("is_staff_or_superuser", is_admin_or_superuser_check) return enforcer diff --git a/openedx_authz/settings/common.py b/openedx_authz/settings/common.py index d737736a..32dc2384 100644 --- a/openedx_authz/settings/common.py +++ b/openedx_authz/settings/common.py @@ -31,6 +31,8 @@ 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 = 0 diff --git a/openedx_authz/tests/test_enforcer.py b/openedx_authz/tests/test_enforcer.py index ae186e99..432cc36a 100644 --- a/openedx_authz/tests/test_enforcer.py +++ b/openedx_authz/tests/test_enforcer.py @@ -825,6 +825,7 @@ def test_multiple_get_enforcer_calls_preserve_auto_save(self, mock_toggle): AuthzEnforcer.get_enforcer() self.assertTrue(AuthzEnforcer.is_auto_save_enabled()) + class TestEnforcerPolicyCacheBehavior(TransactionTestCase): """Test cases for enforcer policy cache behavior. From 67fbbdb751e627c75cea7078f41bc5afee278a9e Mon Sep 17 00:00:00 2001 From: Rodrigo Mendez Date: Thu, 13 Nov 2025 16:39:57 -0600 Subject: [PATCH 08/12] squash!: Changed approach to use DB instead of cache --- openedx_authz/engine/enforcer.py | 17 +++---- .../migrations/0005_policycachecontrol.py | 21 ++++++++ openedx_authz/models/engine.py | 49 +++++++++++++++++++ openedx_authz/tests/test_enforcer.py | 13 +++-- 4 files changed, 83 insertions(+), 17 deletions(-) create mode 100644 openedx_authz/migrations/0005_policycachecontrol.py create mode 100644 openedx_authz/models/engine.py diff --git a/openedx_authz/engine/enforcer.py b/openedx_authz/engine/enforcer.py index 4c7c0702..f300cc4b 100644 --- a/openedx_authz/engine/enforcer.py +++ b/openedx_authz/engine/enforcer.py @@ -21,9 +21,9 @@ from casbin import SyncedEnforcer from casbin_adapter.enforcer import initialize_enforcer from django.conf import settings -from django.core.cache import cache from openedx_authz.engine.adapter import ExtendedAdapter +from openedx_authz.models.engine import PolicyCacheControl def libraries_v2_enabled() -> bool: @@ -68,8 +68,6 @@ class AuthzEnforcer: _adapter (ExtendedAdapter): The singleton adapter instance. """ - CACHE_KEY = "authz_policy_last_modified_timestamp" - _enforcer = None _adapter = None _last_policy_load_timestamp = None @@ -151,7 +149,6 @@ def configure_enforcer_auto_save_and_load(cls): auto_load_policy_interval = getattr(settings, "CASBIN_AUTO_LOAD_POLICY_INTERVAL", 0) auto_save_policy = getattr(settings, "CASBIN_AUTO_SAVE_POLICY", True) - # TODO: remove autoload in favor of cache invalidation? if auto_load_policy_interval > 0: cls.configure_enforcer_auto_loading(auto_load_policy_interval) else: @@ -170,20 +167,20 @@ def load_policy_if_needed(cls): Returns: None """ - last_modified_timestamp = cache.get(cls.CACHE_KEY) + last_modified_timestamp = PolicyCacheControl.get_last_modified_timestamp() current_timestamp = time.time() if last_modified_timestamp is None: # No timestamp in cache; initialize it - cache.set(cls.CACHE_KEY, current_timestamp, None) - logger.info(f">>>> Initialized policy last modified timestamp in cache. {current_timestamp}") + PolicyCacheControl.set_last_modified_timestamp(current_timestamp) + logger.info("Initialized policy last modified timestamp in cache control.") if cls._last_policy_load_timestamp is None or last_modified_timestamp > cls._last_policy_load_timestamp: # Policy has been modified since last load; reload it cls._enforcer.load_policy() cls._last_policy_load_timestamp = current_timestamp - logger.info(f">>>> Reloaded policy at {current_timestamp}") + logger.info(f"Reloaded policy at {current_timestamp}") @classmethod def invalidate_policy_cache(cls): @@ -196,8 +193,8 @@ def invalidate_policy_cache(cls): None """ current_timestamp = time.time() - cache.set(cls.CACHE_KEY, current_timestamp, None) - logger.info(f">>>> Invalidated policy cache at {current_timestamp}") + PolicyCacheControl.set_last_modified_timestamp(current_timestamp) + logger.info(f"Invalidated policy cache at {current_timestamp}") @classmethod def get_enforcer(cls) -> SyncedEnforcer: diff --git a/openedx_authz/migrations/0005_policycachecontrol.py b/openedx_authz/migrations/0005_policycachecontrol.py new file mode 100644 index 00000000..c6b1737f --- /dev/null +++ b/openedx_authz/migrations/0005_policycachecontrol.py @@ -0,0 +1,21 @@ +# Generated by Django 4.2.24 on 2025-11-13 22:32 + +import datetime + +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")), + ("last_modified", models.DateTimeField(default=datetime.datetime.now)), + ], + ), + ] diff --git a/openedx_authz/models/engine.py b/openedx_authz/models/engine.py new file mode 100644 index 00000000..bf3c456c --- /dev/null +++ b/openedx_authz/models/engine.py @@ -0,0 +1,49 @@ +"""Models for the authorization engine.""" + +from datetime import datetime + +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 updating its timestamp. Whenever this model is updated, the authorization + engine should invalidate its cached policies. + """ + + last_modified = models.DateTimeField(default=datetime.now) + + def save(self, *args, **kwargs): + """Override save to update the timestamp.""" + 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_last_modified_timestamp(cls): + """Get the last modified timestamp for policy cache control. + + Returns: + float: The timestamp of the last update. + """ + instance = cls.get() + return instance.last_modified.timestamp() + + @classmethod + def set_last_modified_timestamp(cls, timestamp: float): + """Update the last modified timestamp to the current time. + + This method updates the timestamp, which can be used to signal + that the policy cache should be invalidated. + """ + instance = cls.get() + instance.last_modified = datetime.fromtimestamp(timestamp) + + instance.save() diff --git a/openedx_authz/tests/test_enforcer.py b/openedx_authz/tests/test_enforcer.py index 432cc36a..76e9f0d4 100644 --- a/openedx_authz/tests/test_enforcer.py +++ b/openedx_authz/tests/test_enforcer.py @@ -12,12 +12,12 @@ from ddt import data as ddt_data from ddt import ddt from django.conf import settings -from django.core.cache import cache from django.test import TestCase, TransactionTestCase, override_settings 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 @@ -845,12 +845,11 @@ def test_load_policy_if_needed_initializes_cache_timestamp(self, mock_toggle): mock_toggle.return_value = True AuthzEnforcer._last_policy_load_timestamp = None # pylint: disable=protected-access - cache.clear() # get_enforcer calls load_policy_if_needed internally AuthzEnforcer.get_enforcer() - cached_timestamp = cache.get(AuthzEnforcer.CACHE_KEY) + cached_timestamp = PolicyCacheControl.get_last_modified_timestamp() self.assertIsNotNone(cached_timestamp) self.assertIsNotNone(AuthzEnforcer._last_policy_load_timestamp) # pylint: disable=protected-access @@ -871,7 +870,7 @@ def test_load_policy_if_needed_loads_when_stale(self, mock_toggle): # Set last load timestamp to stale value AuthzEnforcer._last_policy_load_timestamp = stale_timestamp # pylint: disable=protected-access # Set last cache invalidation to a more recent time - cache.set(AuthzEnforcer.CACHE_KEY, now, None) + PolicyCacheControl.set_last_modified_timestamp(now) # get_enforcer calls load_policy_if_needed internally AuthzEnforcer.get_enforcer() @@ -898,7 +897,7 @@ def test_load_policy_if_needed_doesnt_reload_when_not_stale(self, mock_toggle): # Set last load timestamp to current time AuthzEnforcer._last_policy_load_timestamp = now # pylint: disable=protected-access # Set last cache invalidation to an earlier time - cache.set(AuthzEnforcer.CACHE_KEY, now - 60, None) # 60 seconds ago + PolicyCacheControl.set_last_modified_timestamp(now - 60) # 60 seconds ago # get_enforcer calls load_policy_if_needed internally AuthzEnforcer.get_enforcer() @@ -920,10 +919,10 @@ def test_invalidate_policy_cache(self, mock_toggle): AuthzEnforcer._last_policy_load_timestamp = time.time() # pylint: disable=protected-access old_cache_value = time.time() - 60 # 60 seconds ago - cache.set(AuthzEnforcer.CACHE_KEY, old_cache_value, None) + PolicyCacheControl.set_last_modified_timestamp(old_cache_value) AuthzEnforcer.invalidate_policy_cache() - new_cache_value = cache.get(AuthzEnforcer.CACHE_KEY) + new_cache_value = PolicyCacheControl.get_last_modified_timestamp() self.assertIsNotNone(new_cache_value) self.assertGreater(new_cache_value, old_cache_value) From 8da767bc473825770aba2b8357c75469de5c17d7 Mon Sep 17 00:00:00 2001 From: Rodrigo Mendez Date: Fri, 14 Nov 2025 08:44:37 -0600 Subject: [PATCH 09/12] squash!: Add tests for PolicyCacheControl model --- openedx_authz/tests/test_models.py | 37 ++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/openedx_authz/tests/test_models.py b/openedx_authz/tests/test_models.py index 4bdf77ca..fdeb050d 100644 --- a/openedx_authz/tests/test_models.py +++ b/openedx_authz/tests/test_models.py @@ -20,6 +20,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 +137,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_last_modified_timestamp(self): + """Test getting and setting the last modified timestamp. + + Expected Result: + - Initially, the timestamp is set to the current time. + - After setting a new timestamp, it reflects the updated value. + """ + initial_timestamp = PolicyCacheControl.get_last_modified_timestamp() + self.assertIsInstance(initial_timestamp, float) + + new_timestamp = initial_timestamp + 1000.0 # Simulate a future timestamp + PolicyCacheControl.set_last_modified_timestamp(new_timestamp) + + updated_timestamp = PolicyCacheControl.get_last_modified_timestamp() + self.assertEqual(updated_timestamp, new_timestamp) + + 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) From 5f03619312628b7d94c4f6575b089da886c4ec2e Mon Sep 17 00:00:00 2001 From: Rodrigo Mendez Date: Fri, 14 Nov 2025 16:56:47 -0600 Subject: [PATCH 10/12] squash!: Changed implementation to use UUID for cache invalidation --- openedx_authz/engine/enforcer.py | 35 +++++----- .../migrations/0005_policycachecontrol.py | 6 +- openedx_authz/models/engine.py | 24 +++---- openedx_authz/tests/api/test_roles.py | 1 - openedx_authz/tests/test_enforcer.py | 70 ++++++++++--------- openedx_authz/tests/test_models.py | 22 +++--- 6 files changed, 81 insertions(+), 77 deletions(-) diff --git a/openedx_authz/engine/enforcer.py b/openedx_authz/engine/enforcer.py index f300cc4b..e04cc5f6 100644 --- a/openedx_authz/engine/enforcer.py +++ b/openedx_authz/engine/enforcer.py @@ -16,7 +16,7 @@ """ import logging -import time +from uuid import uuid4 from casbin import SyncedEnforcer from casbin_adapter.enforcer import initialize_enforcer @@ -70,7 +70,7 @@ class AuthzEnforcer: _enforcer = None _adapter = None - _last_policy_load_timestamp = None + _last_policy_loaded_version = None def __new__(cls): """Singleton pattern to ensure a single enforcer instance.""" @@ -158,43 +158,42 @@ def configure_enforcer_auto_save_and_load(cls): @classmethod def load_policy_if_needed(cls): - """Load policy if the last load timestamp indicates it's needed. + """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 timestamp with the last modified timestamp in cache + the last load version with the version in the cache invalidation model, and reloads it if necessary. Returns: None """ - last_modified_timestamp = PolicyCacheControl.get_last_modified_timestamp() + last_version = PolicyCacheControl.get_version() - current_timestamp = time.time() - - if last_modified_timestamp is None: + if last_version is None: # No timestamp in cache; initialize it - PolicyCacheControl.set_last_modified_timestamp(current_timestamp) - logger.info("Initialized policy last modified timestamp in cache control.") + last_version = uuid4() + PolicyCacheControl.set_version(last_version) + logger.info("Initialized policy last modified version in cache control.") - if cls._last_policy_load_timestamp is None or last_modified_timestamp > cls._last_policy_load_timestamp: + 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_load_timestamp = current_timestamp - logger.info(f"Reloaded policy at {current_timestamp}") + 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 timestamp in the cache to - the current time, indicating that the policy has changed. + This method updates the last modified version in the cache invalidation model + to a new UUID, indicating that the policy has changed. Returns: None """ - current_timestamp = time.time() - PolicyCacheControl.set_last_modified_timestamp(current_timestamp) - logger.info(f"Invalidated policy cache at {current_timestamp}") + new_version = uuid4() + PolicyCacheControl.set_version(new_version) + logger.info(f"Invalidated policy cache to version {new_version}") @classmethod def get_enforcer(cls) -> SyncedEnforcer: diff --git a/openedx_authz/migrations/0005_policycachecontrol.py b/openedx_authz/migrations/0005_policycachecontrol.py index c6b1737f..98ccf115 100644 --- a/openedx_authz/migrations/0005_policycachecontrol.py +++ b/openedx_authz/migrations/0005_policycachecontrol.py @@ -1,6 +1,6 @@ -# Generated by Django 4.2.24 on 2025-11-13 22:32 +# Generated by Django 4.2.24 on 2025-11-14 22:38 -import datetime +import uuid from django.db import migrations, models @@ -15,7 +15,7 @@ class Migration(migrations.Migration): name="PolicyCacheControl", fields=[ ("id", models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), - ("last_modified", models.DateTimeField(default=datetime.datetime.now)), + ("version", models.UUIDField(default=uuid.uuid4)), ], ), ] diff --git a/openedx_authz/models/engine.py b/openedx_authz/models/engine.py index bf3c456c..3060f4da 100644 --- a/openedx_authz/models/engine.py +++ b/openedx_authz/models/engine.py @@ -1,6 +1,6 @@ """Models for the authorization engine.""" -from datetime import datetime +from uuid import UUID, uuid4 from django.db import models @@ -9,14 +9,14 @@ class PolicyCacheControl(models.Model): """Model to control policy cache invalidation. This model can be used to trigger cache invalidation for authorization policies - by updating its timestamp. Whenever this model is updated, the authorization + by changing the version. Whenever this model is updated, the authorization engine should invalidate its cached policies. """ - last_modified = models.DateTimeField(default=datetime.now) + version = models.UUIDField(default=uuid4) def save(self, *args, **kwargs): - """Override save to update the timestamp.""" + """Override save to ensure a single instance.""" self.pk = 1 # Ensure a single instance super().save(*args, **kwargs) @@ -27,23 +27,23 @@ def get(cls): return obj @classmethod - def get_last_modified_timestamp(cls): - """Get the last modified timestamp for policy cache control. + def get_version(cls): + """Get the version for policy cache control. Returns: - float: The timestamp of the last update. + UUID: The version of the last update. """ instance = cls.get() - return instance.last_modified.timestamp() + return instance.version @classmethod - def set_last_modified_timestamp(cls, timestamp: float): - """Update the last modified timestamp to the current time. + def set_version(cls, version: UUID): + """Update the cache version. - This method updates the timestamp, which can be used to signal + This method updates the cache version, which can be used to signal that the policy cache should be invalidated. """ instance = cls.get() - instance.last_modified = datetime.fromtimestamp(timestamp) + instance.version = version instance.save() 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 76e9f0d4..25ceab46 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 @@ -839,19 +840,22 @@ def test_load_policy_if_needed_initializes_cache_timestamp(self, mock_toggle): """Test that load_policy_if_needed initializes cache timestamp on first call. Expected result: - - On first call, cache invalidation key is set with current timestamp - - Policy is loaded since last load timestamp is None + - 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_load_timestamp = None # pylint: disable=protected-access - + AuthzEnforcer._last_policy_loaded_version = None # pylint: disable=protected-access # get_enforcer calls load_policy_if_needed internally AuthzEnforcer.get_enforcer() - cached_timestamp = PolicyCacheControl.get_last_modified_timestamp() - self.assertIsNotNone(cached_timestamp) - self.assertIsNotNone(AuthzEnforcer._last_policy_load_timestamp) # pylint: disable=protected-access + 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) @@ -860,25 +864,25 @@ def test_load_policy_if_needed_loads_when_stale(self, mock_toggle): Expected result: - If policy is stale, it is reloaded - - _last_policy_load_timestamp is updated with new timestamp + - _last_policy_loaded_version is updated with new version """ mock_toggle.return_value = True - now = time.time() - stale_timestamp = now - 60 # 60 seconds ago + stale_version = uuid4() + current_version = uuid4() - # Set last load timestamp to stale value - AuthzEnforcer._last_policy_load_timestamp = stale_timestamp # pylint: disable=protected-access - # Set last cache invalidation to a more recent time - PolicyCacheControl.set_last_modified_timestamp(now) + # 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_load_timestamp) # pylint: disable=protected-access - self.assertGreater( - AuthzEnforcer._last_policy_load_timestamp, # pylint: disable=protected-access - stale_timestamp, + 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") @@ -888,41 +892,41 @@ def test_load_policy_if_needed_doesnt_reload_when_not_stale(self, mock_toggle): Expected result: - If policy is not stale, it is not reloaded - - _last_policy_load_timestamp remains unchanged + - _last_policy_loaded_version remains unchanged """ mock_toggle.return_value = True - now = time.time() + current_version = uuid4() - # Set last load timestamp to current time - AuthzEnforcer._last_policy_load_timestamp = now # pylint: disable=protected-access - # Set last cache invalidation to an earlier time - PolicyCacheControl.set_last_modified_timestamp(now - 60) # 60 seconds ago + # 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_load_timestamp, # pylint: disable=protected-access - now, + 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 key. + """Test that invalidate_policy_cache updates the cache invalidation model. Expected result: - - Cache invalidation key is updated with current timestamp + - Cache invalidation key is updated to a new version """ mock_toggle.return_value = True - AuthzEnforcer._last_policy_load_timestamp = time.time() # pylint: disable=protected-access - old_cache_value = time.time() - 60 # 60 seconds ago - PolicyCacheControl.set_last_modified_timestamp(old_cache_value) + 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_last_modified_timestamp() + new_cache_value = PolicyCacheControl.get_version() self.assertIsNotNone(new_cache_value) - self.assertGreater(new_cache_value, old_cache_value) + self.assertNotEqual(new_cache_value, old_cache_value) diff --git a/openedx_authz/tests/test_models.py b/openedx_authz/tests/test_models.py index fdeb050d..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 @@ -142,21 +144,21 @@ def test_extended_casbin_rule_cascade_deletion_when_subject_deleted(self): class TestPolicyCacheControlModel(TestCase): """Test cases for the PolicyCacheControl model.""" - def test_get_and_set_last_modified_timestamp(self): - """Test getting and setting the last modified timestamp. + def test_get_and_set_version(self): + """Test getting and setting the cache version. Expected Result: - - Initially, the timestamp is set to the current time. - - After setting a new timestamp, it reflects the updated value. + - Initially, the version is set to a UUID. + - After setting a new version, it reflects the updated value. """ - initial_timestamp = PolicyCacheControl.get_last_modified_timestamp() - self.assertIsInstance(initial_timestamp, float) + initial_version = PolicyCacheControl.get_version() + self.assertIsInstance(initial_version, UUID) - new_timestamp = initial_timestamp + 1000.0 # Simulate a future timestamp - PolicyCacheControl.set_last_modified_timestamp(new_timestamp) + new_version = uuid4() # Generate a new UUID + PolicyCacheControl.set_version(new_version) - updated_timestamp = PolicyCacheControl.get_last_modified_timestamp() - self.assertEqual(updated_timestamp, new_timestamp) + updated_version = PolicyCacheControl.get_version() + self.assertEqual(updated_version, new_version) def test_singleton_behavior(self): """Test that only one instance of PolicyCacheControl exists. From b7f31cc3f4ad86eff2c29742c9692cb3137e889f Mon Sep 17 00:00:00 2001 From: Rodrigo Mendez Date: Tue, 18 Nov 2025 09:49:50 -0600 Subject: [PATCH 11/12] squash!: Attend PR comments --- openedx_authz/engine/enforcer.py | 2 +- openedx_authz/models/engine.py | 2 ++ openedx_authz/tests/test_enforcer.py | 4 ++-- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/openedx_authz/engine/enforcer.py b/openedx_authz/engine/enforcer.py index e04cc5f6..a0caad9a 100644 --- a/openedx_authz/engine/enforcer.py +++ b/openedx_authz/engine/enforcer.py @@ -170,7 +170,7 @@ def load_policy_if_needed(cls): last_version = PolicyCacheControl.get_version() if last_version is None: - # No timestamp in cache; initialize it + # 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.") diff --git a/openedx_authz/models/engine.py b/openedx_authz/models/engine.py index 3060f4da..cc6f8bc4 100644 --- a/openedx_authz/models/engine.py +++ b/openedx_authz/models/engine.py @@ -11,6 +11,8 @@ class PolicyCacheControl(models.Model): 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) diff --git a/openedx_authz/tests/test_enforcer.py b/openedx_authz/tests/test_enforcer.py index 25ceab46..ea12d855 100644 --- a/openedx_authz/tests/test_enforcer.py +++ b/openedx_authz/tests/test_enforcer.py @@ -836,8 +836,8 @@ class TestEnforcerPolicyCacheBehavior(TransactionTestCase): @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0) - def test_load_policy_if_needed_initializes_cache_timestamp(self, mock_toggle): - """Test that load_policy_if_needed initializes cache timestamp on first call. + 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 From de47a7d9ad71846d62a7bb79b243eb6eac628ca3 Mon Sep 17 00:00:00 2001 From: Rodrigo Mendez Date: Tue, 18 Nov 2025 10:01:58 -0600 Subject: [PATCH 12/12] squash!: Fix migrations conflict --- ....py => 0006_migrate_legacy_permissions.py} | 2 +- openedx_authz/tests/test_imports.py | 21 ++++++++++--------- 2 files changed, 12 insertions(+), 11 deletions(-) rename openedx_authz/migrations/{0005_migrate_legacy_permissions.py => 0006_migrate_legacy_permissions.py} (97%) 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/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: