From ee344ff13d2109fa49549287835d1a696dd59607 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 28 Oct 2025 19:52:19 +0100 Subject: [PATCH 1/7] refactor: disable enforcer auto save when content libraries V2 is off --- openedx_authz/engine/enforcer.py | 68 +++++++++++++++++++++++++++----- 1 file changed, 59 insertions(+), 9 deletions(-) diff --git a/openedx_authz/engine/enforcer.py b/openedx_authz/engine/enforcer.py index cc591276..89dd33b0 100644 --- a/openedx_authz/engine/enforcer.py +++ b/openedx_authz/engine/enforcer.py @@ -23,6 +23,16 @@ from openedx_authz.engine.adapter import ExtendedAdapter +try: + from cms.djangoapps.contentstore.toggles import libraries_v2_enabled +except ImportError: + # If the CMS is not available, define a dummy toggle that is always enabled + class DummyToggle: + @staticmethod + def is_enabled(): + return True + + logger = logging.getLogger(__name__) @@ -57,6 +67,44 @@ def __new__(cls): cls._enforcer = cls._initialize_enforcer() return cls._enforcer + @classmethod + def deactivate_enforcer(cls): + """Deactivate the current enforcer instance, if any. + + This method stops the auto-load policy thread. It can be used in testing + or when re-initialization of the enforcer is needed. IT DOES NOT + clear the singleton instance to avoid initializing it again unintentionally. + + Returns: + None + """ + if cls._enforcer is not None: + try: + cls._enforcer.stop_auto_load_policy() + cls._enforcer.enable_auto_save(False) + except Exception as e: + logger.error(f"Error stopping auto-load policy thread: {e}") + + @classmethod + def enable_enforcer_auto_save_and_load(cls): + """Enable auto-load policy and auto-save on the enforcer. + + This method ensures that the singleton enforcer instance is created + and ready for use. + + Returns: + None + """ + auto_load_policy_interval = getattr(settings, "CASBIN_AUTO_LOAD_POLICY_INTERVAL", 0) + + if auto_load_policy_interval > 0: + cls._enforcer.start_auto_load_policy(auto_load_policy_interval) + cls._enforcer.enable_auto_save(True) + else: + # Disable auto-save to prevent unnecessary database writes + cls._enforcer.enable_auto_save(False) + logger.warning("CASBIN_AUTO_LOAD_POLICY_INTERVAL is not set or zero; auto-load is disabled.") + @classmethod def get_enforcer(cls) -> SyncedEnforcer: """Get the enforcer instance, creating it if needed. @@ -66,10 +114,17 @@ def get_enforcer(cls) -> SyncedEnforcer: """ if cls._enforcer is None: cls._enforcer = cls._initialize_enforcer() + + # 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. + if not libraries_v2_enabled.is_enabled(): + cls.deactivate_enforcer() + return cls._enforcer - @staticmethod - def _initialize_enforcer() -> SyncedEnforcer: + @classmethod + def _initialize_enforcer(cls) -> SyncedEnforcer: """ Create and configure the Casbin SyncedEnforcer instance. @@ -93,12 +148,7 @@ def _initialize_enforcer() -> SyncedEnforcer: adapter = ExtendedAdapter() enforcer = SyncedEnforcer(settings.CASBIN_MODEL, adapter) - auto_load_policy_interval = getattr(settings, "CASBIN_AUTO_LOAD_POLICY_INTERVAL", 0) - if auto_load_policy_interval > 0: - enforcer.start_auto_load_policy(auto_load_policy_interval) - enforcer.enable_auto_save(True) - else: - # Disable auto-save to prevent unnecessary database writes - enforcer.enable_auto_save(False) + + cls.enable_enforcer_auto_save_and_load() return enforcer From c561462c8e8ab9914ec324681dbf085861213dc0 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 28 Oct 2025 20:04:56 +0100 Subject: [PATCH 2/7] refactor: address quality issues --- openedx_authz/engine/enforcer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_authz/engine/enforcer.py b/openedx_authz/engine/enforcer.py index 89dd33b0..55b6b9a6 100644 --- a/openedx_authz/engine/enforcer.py +++ b/openedx_authz/engine/enforcer.py @@ -82,7 +82,7 @@ def deactivate_enforcer(cls): try: cls._enforcer.stop_auto_load_policy() cls._enforcer.enable_auto_save(False) - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught logger.error(f"Error stopping auto-load policy thread: {e}") @classmethod From 7e2c98a4fbb623c0422fede0d4f2a1115898166d Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 28 Oct 2025 20:13:52 +0100 Subject: [PATCH 3/7] refactor: move enable enforcer after initializing --- openedx_authz/engine/enforcer.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openedx_authz/engine/enforcer.py b/openedx_authz/engine/enforcer.py index 55b6b9a6..e962a6c0 100644 --- a/openedx_authz/engine/enforcer.py +++ b/openedx_authz/engine/enforcer.py @@ -31,6 +31,7 @@ class DummyToggle: @staticmethod def is_enabled(): return True + libraries_v2_enabled = DummyToggle() logger = logging.getLogger(__name__) @@ -65,6 +66,7 @@ def __new__(cls): """Singleton pattern to ensure a single enforcer instance.""" if cls._enforcer is None: cls._enforcer = cls._initialize_enforcer() + cls.enable_enforcer_auto_save_and_load() return cls._enforcer @classmethod @@ -114,6 +116,7 @@ def get_enforcer(cls) -> SyncedEnforcer: """ if cls._enforcer is None: cls._enforcer = cls._initialize_enforcer() + cls.enable_enforcer_auto_save_and_load() # 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 @@ -149,6 +152,4 @@ def _initialize_enforcer(cls) -> SyncedEnforcer: adapter = ExtendedAdapter() enforcer = SyncedEnforcer(settings.CASBIN_MODEL, adapter) - cls.enable_enforcer_auto_save_and_load() - return enforcer From 6135e6e07e9adb21318083f9885a30af1fe96441 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Tue, 28 Oct 2025 20:41:28 +0100 Subject: [PATCH 4/7] refactor: consider auto loading only if not already --- openedx_authz/engine/enforcer.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/openedx_authz/engine/enforcer.py b/openedx_authz/engine/enforcer.py index e962a6c0..1eba2a21 100644 --- a/openedx_authz/engine/enforcer.py +++ b/openedx_authz/engine/enforcer.py @@ -66,7 +66,6 @@ def __new__(cls): """Singleton pattern to ensure a single enforcer instance.""" if cls._enforcer is None: cls._enforcer = cls._initialize_enforcer() - cls.enable_enforcer_auto_save_and_load() return cls._enforcer @classmethod @@ -99,14 +98,15 @@ def enable_enforcer_auto_save_and_load(cls): """ auto_load_policy_interval = getattr(settings, "CASBIN_AUTO_LOAD_POLICY_INTERVAL", 0) + # Only start auto-load if it's not already running if auto_load_policy_interval > 0: - cls._enforcer.start_auto_load_policy(auto_load_policy_interval) - cls._enforcer.enable_auto_save(True) + if not cls._enforcer.is_auto_loading_running(): + cls._enforcer.start_auto_load_policy(auto_load_policy_interval) else: - # Disable auto-save to prevent unnecessary database writes - cls._enforcer.enable_auto_save(False) logger.warning("CASBIN_AUTO_LOAD_POLICY_INTERVAL is not set or zero; auto-load is disabled.") + cls._enforcer.enable_auto_save(True) + @classmethod def get_enforcer(cls) -> SyncedEnforcer: """Get the enforcer instance, creating it if needed. @@ -116,12 +116,13 @@ def get_enforcer(cls) -> SyncedEnforcer: """ if cls._enforcer is None: cls._enforcer = cls._initialize_enforcer() - cls.enable_enforcer_auto_save_and_load() # 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. - if not libraries_v2_enabled.is_enabled(): + if libraries_v2_enabled.is_enabled(): + cls.enable_enforcer_auto_save_and_load() + else: cls.deactivate_enforcer() return cls._enforcer From 0b7af8fb0175369360fc692c9b20f51fd9a15bb8 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 29 Oct 2025 13:05:43 +0100 Subject: [PATCH 5/7] refactor: configure auto-load and save separately --- openedx_authz/engine/enforcer.py | 47 ++++- openedx_authz/settings/common.py | 19 +- openedx_authz/settings/test.py | 1 + openedx_authz/tests/test_enforcer.py | 296 +++++++++++++++++++++++++-- 4 files changed, 338 insertions(+), 25 deletions(-) diff --git a/openedx_authz/engine/enforcer.py b/openedx_authz/engine/enforcer.py index 1eba2a21..112433d6 100644 --- a/openedx_authz/engine/enforcer.py +++ b/openedx_authz/engine/enforcer.py @@ -31,6 +31,7 @@ class DummyToggle: @staticmethod def is_enabled(): return True + libraries_v2_enabled = DummyToggle() @@ -68,6 +69,35 @@ def __new__(cls): cls._enforcer = cls._initialize_enforcer() return cls._enforcer + @classmethod + def configure_enforcer_auto_loading(cls, auto_load_policy_interval: int = None): + """Enable auto-load policy and auto-save on the enforcer. + + This method ensures that the singleton enforcer instance is created + and ready for use. + + Returns: + None + """ + if not cls._enforcer.is_auto_loading_running(): + cls._enforcer.start_auto_load_policy(auto_load_policy_interval) + + @classmethod + def configure_enforcer_auto_save(cls, auto_save_policy: bool): + """Configure auto-save on the enforcer. + + This method ensures that auto-save is enabled or disabled based on + the auto_save_policy parameter. + + Args: + auto_save_policy: True to enable auto-save, False to disable + + Returns: + None + """ + if cls._enforcer._e.auto_save != auto_save_policy: + cls._enforcer.enable_auto_save(auto_save_policy) + @classmethod def deactivate_enforcer(cls): """Deactivate the current enforcer instance, if any. @@ -87,25 +117,24 @@ def deactivate_enforcer(cls): logger.error(f"Error stopping auto-load policy thread: {e}") @classmethod - def enable_enforcer_auto_save_and_load(cls): + def configure_enforcer_auto_save_and_load(cls): """Enable auto-load policy and auto-save on the enforcer. - This method ensures that the singleton enforcer instance is created - and ready for use. + This method ensures that the singleton enforcer instance is configured + for auto-load and auto-save based on settings. Returns: None """ auto_load_policy_interval = getattr(settings, "CASBIN_AUTO_LOAD_POLICY_INTERVAL", 0) + auto_save_policy = getattr(settings, "CASBIN_AUTO_SAVE_POLICY", True) - # Only start auto-load if it's not already running if auto_load_policy_interval > 0: - if not cls._enforcer.is_auto_loading_running(): - cls._enforcer.start_auto_load_policy(auto_load_policy_interval) + 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._enforcer.enable_auto_save(True) + cls.configure_enforcer_auto_save(auto_save_policy) @classmethod def get_enforcer(cls) -> SyncedEnforcer: @@ -120,8 +149,10 @@ def get_enforcer(cls) -> SyncedEnforcer: # 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. + # When replaced, we will only need to configure the enforcer here. Which + # is in charge of enabling/disabling auto-load and auto-save. if libraries_v2_enabled.is_enabled(): - cls.enable_enforcer_auto_save_and_load() + cls.configure_enforcer_auto_save_and_load() else: cls.deactivate_enforcer() diff --git a/openedx_authz/settings/common.py b/openedx_authz/settings/common.py index 0de26897..c3c2840e 100644 --- a/openedx_authz/settings/common.py +++ b/openedx_authz/settings/common.py @@ -20,7 +20,22 @@ def plugin_settings(settings): casbin_adapter_app = "openedx_authz.engine.apps.CasbinAdapterConfig" if casbin_adapter_app not in settings.INSTALLED_APPS: settings.INSTALLED_APPS.append(casbin_adapter_app) - # Add Casbin configuration - settings.CASBIN_MODEL = os.path.join(ROOT_DIRECTORY, "engine", "config", "model.conf") + + # Casbin settings for model and policy synchronization + + # Set default CASBIN_MODEL if not already set, this points to the model.conf file + # which defines the access control model for Casbin. + if not hasattr(settings, "CASBIN_MODEL"): + settings.CASBIN_MODEL = os.path.join(ROOT_DIRECTORY, "engine", "config", "model.conf") + + # 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. if not hasattr(settings, "CASBIN_AUTO_LOAD_POLICY_INTERVAL"): settings.CASBIN_AUTO_LOAD_POLICY_INTERVAL = 5 + + # Set default CASBIN_AUTO_SAVE_POLICY if not already set. + # This setting defines whether the Casbin enforcer should automatically + # save policy changes back to the database. + if not hasattr(settings, "CASBIN_AUTO_SAVE_POLICY"): + settings.CASBIN_AUTO_SAVE_POLICY = True diff --git a/openedx_authz/settings/test.py b/openedx_authz/settings/test.py index c187760f..42a79c75 100644 --- a/openedx_authz/settings/test.py +++ b/openedx_authz/settings/test.py @@ -70,3 +70,4 @@ def plugin_settings(settings): # pylint: disable=unused-argument # Casbin configuration CASBIN_MODEL = os.path.join(ROOT_DIRECTORY, "engine", "config", "model.conf") CASBIN_AUTO_LOAD_POLICY_INTERVAL = 0 +CASBIN_AUTO_SAVE_POLICY = True diff --git a/openedx_authz/tests/test_enforcer.py b/openedx_authz/tests/test_enforcer.py index 878a3c3c..84c49f4c 100644 --- a/openedx_authz/tests/test_enforcer.py +++ b/openedx_authz/tests/test_enforcer.py @@ -6,6 +6,7 @@ """ import time +from unittest.mock import Mock, patch import casbin from ddt import data as ddt_data @@ -521,39 +522,304 @@ def test_auto_load_policy_detects_changes(self): policies_after_auto_load = global_enforcer.get_grouping_policy() self.assertIn(new_assignment, policies_after_auto_load) - @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=-1) + @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0) def test_auto_load_disabled(self): - """Test that auto-load can be disabled by setting interval to -1. + """Test that auto-load can be disabled while auto-save remains enabled. - This test verifies that when CASBIN_AUTO_LOAD_POLICY_INTERVAL is set to -1, - the enforcer does NOT automatically load policies from the database. + This test verifies that when CASBIN_AUTO_LOAD_POLICY_INTERVAL is 0, + the enforcer does NOT automatically load policies, but auto-save + works normally for manual operations. Expected result: - - Policies remain empty after seeding database - - Manual load_policy() is required to load policies - - New policies don't appear without manual reload + - Policies remain empty initially (no auto-load) + - Policies can be seeded to database (auto-save works) + - Manual load_policy() loads policies from database """ global_enforcer = AuthzEnforcer.get_enforcer() - # Initial policy count should be 0 initial_policy_count = len(global_enforcer.get_policy()) self.assertEqual(initial_policy_count, 0) - # Policies should still be empty since auto-load is disabled - # and no database queries should have been made with self.assertNumQueries(0): time.sleep(1.0) policies_after_wait = global_enforcer.get_policy() self.assertEqual(len(policies_after_wait), 0) - # Seed the database with policies self._seed_database_with_policies() - # Manually load policies with self.assertNumQueries(1): time.sleep(1.0) global_enforcer.load_policy() - # Since auto-save is also disabled, the policies should still - # be empty after manual load policies_after_manual_load = global_enforcer.get_policy() - self.assertEqual(len(policies_after_manual_load), 0) + self.assertGreater(len(policies_after_manual_load), 0) + + +class TestEnforcerToggleBehavior(TransactionTestCase): + """Test cases for enforcer behavior with libraries_v2_enabled toggle. + + These tests verify that the enforcer correctly responds to the + libraries_v2_enabled toggle state, enabling/disabling auto-save + and auto-load as appropriate. + + Uses TransactionTestCase to ensure clean state between tests. + """ + + def setUp(self): + """Set up test environment with clean enforcer state.""" + super().setUp() + # Reset the singleton enforcer before each test + AuthzEnforcer._enforcer = None # pylint: disable=protected-access + + def tearDown(self): + """Clean up enforcer state after test.""" + if AuthzEnforcer._enforcer: # pylint: disable=protected-access + try: + AuthzEnforcer.deactivate_enforcer() + except Exception: # pylint: disable=broad-exception-caught + pass + AuthzEnforcer._enforcer = None # pylint: disable=protected-access + super().tearDown() + + @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") + @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0) + def test_enforcer_auto_save_enabled_when_toggle_enabled(self, mock_toggle): + """Test that auto-save is enabled when libraries_v2_enabled toggle is on. + + Expected result: + - Enforcer is initialized with auto-save enabled + - Policy changes are persisted to database + - CASBIN_AUTO_LOAD_POLICY_INTERVAL=0 doesn't disable auto-save + """ + mock_toggle.is_enabled.return_value = True + + enforcer = AuthzEnforcer.get_enforcer() + + self.assertTrue(enforcer._e.auto_save) + + test_policy = [ + make_role_key("test_role"), + make_action_key("test_action"), + make_scope_key("lib", "*"), + "allow", + ] + enforcer.add_policy(*test_policy) + + enforcer.load_policy() + policies = enforcer.get_policy() + + self.assertIn(test_policy, policies) + + @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") + @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0) + def test_enforcer_deactivated_when_toggle_disabled(self, mock_toggle): + """Test that enforcer is deactivated when libraries_v2_enabled toggle is off. + + Expected result: + - Enforcer is initialized but deactivated + - Auto-save is disabled via deactivate_enforcer + - Auto-load is stopped + """ + mock_toggle.is_enabled.return_value = False + + enforcer = AuthzEnforcer.get_enforcer() + + self.assertFalse(enforcer._e.auto_save) + self.assertFalse(enforcer.is_auto_loading_running()) + + @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") + @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0.5) + def test_enforcer_auto_load_starts_when_toggle_enabled(self, mock_toggle): + """Test that auto-load starts when toggle is enabled and interval > 0. + + Expected result: + - Auto-load thread is started with configured interval + - Auto-save is enabled + """ + mock_toggle.is_enabled.return_value = True + + enforcer = AuthzEnforcer.get_enforcer() + + self.assertTrue(enforcer.is_auto_loading_running()) + self.assertTrue(enforcer._e.auto_save) + + @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") + @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0.5) + def test_enforcer_auto_load_not_restarted_on_subsequent_calls(self, mock_toggle): + """Test that auto-load is not restarted on subsequent get_enforcer() calls. + + Expected result: + - Auto-load starts on first call + - Subsequent calls don't restart the auto-load thread + - Auto-save remains enabled + """ + mock_toggle.is_enabled.return_value = True + + enforcer1 = AuthzEnforcer.get_enforcer() + self.assertTrue(enforcer1.is_auto_loading_running()) + + enforcer2 = AuthzEnforcer.get_enforcer() + self.assertIs(enforcer1, enforcer2) + self.assertTrue(enforcer2.is_auto_loading_running()) + self.assertTrue(enforcer2._e.auto_save) + + @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") + @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0) + def test_toggle_state_checked_on_every_get_enforcer_call(self, mock_toggle): + """Test that toggle state is checked on every get_enforcer() call. + + This verifies the "HACK" behavior where the toggle state is + re-evaluated each time get_enforcer() is called. + + Expected result: + - First call with toggle off: auto-save disabled + - Second call with toggle on: auto-save enabled + """ + mock_toggle.is_enabled.return_value = False + enforcer1 = AuthzEnforcer.get_enforcer() + self.assertFalse(enforcer1._e.auto_save) + + mock_toggle.is_enabled.return_value = True + enforcer2 = AuthzEnforcer.get_enforcer() + self.assertIs(enforcer1, enforcer2) + self.assertTrue(enforcer2._e.auto_save) + + @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") + @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0) + def test_dummy_toggle_behavior_in_tests(self, mock_toggle): + """Test enforcer behavior with DummyToggle (CMS not available). + + When CMS is not available, a DummyToggle is used that always + returns True. This test verifies that the enforcer still works + correctly in this scenario. + + Expected result: + - Enforcer initializes successfully + - Auto-save is enabled (DummyToggle returns True) + """ + mock_toggle.is_enabled.return_value = True + + enforcer = AuthzEnforcer.get_enforcer() + + self.assertIsNotNone(enforcer) + self.assertTrue(enforcer._e.auto_save) + + @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") + @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0) + def test_auto_save_preserved_with_interval_zero(self, mock_toggle): + """Test that auto-save state is preserved when interval is 0. + + When CASBIN_AUTO_LOAD_POLICY_INTERVAL is 0, calling get_enforcer() + multiple times should not disable auto-save if it was manually enabled. + + Expected result: + - Tests can manually enable auto-save + - Subsequent get_enforcer() calls preserve auto-save state + """ + mock_toggle.is_enabled.return_value = True + + enforcer = AuthzEnforcer.get_enforcer() + enforcer.enable_auto_save(True) + self.assertTrue(enforcer._e.auto_save) + + # Call get_enforcer() again - should not disable auto-save + enforcer2 = AuthzEnforcer.get_enforcer() + self.assertIs(enforcer, enforcer2) + self.assertTrue(enforcer2._e.auto_save) + + @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") + @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0) + def test_auto_save_persistence_with_interval_zero(self, mock_toggle): + """Test that policies persist to database when auto-save is enabled with interval 0. + + Expected result: + - Policies added via add_policy() are persisted to database + - Policies can be reloaded from database + """ + mock_toggle.is_enabled.return_value = True + + enforcer = AuthzEnforcer.get_enforcer() + enforcer.enable_auto_save(True) + + test_policy = [ + make_role_key("test_role"), + make_action_key("test_action"), + make_scope_key("lib", "*"), + "allow", + ] + enforcer.add_policy(*test_policy) + + # Reload from database + enforcer.load_policy() + policies = enforcer.get_policy() + + self.assertIn(test_policy, policies) + + @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") + @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0, CASBIN_AUTO_SAVE_POLICY=False) + def test_auto_save_disabled_explicitly(self, mock_toggle): + """Test that auto-save is disabled when CASBIN_AUTO_SAVE_POLICY is False. + + Expected result: + - Auto-save is disabled + - Auto-load is not running + """ + mock_toggle.is_enabled.return_value = True + + enforcer = AuthzEnforcer.get_enforcer() + + self.assertFalse(enforcer._e.auto_save) + self.assertFalse(enforcer.is_auto_loading_running()) + + @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") + @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0, CASBIN_AUTO_SAVE_POLICY=False) + def test_policies_not_persisted_when_auto_save_disabled(self, mock_toggle): + """Test that policies don't persist when auto-save is explicitly disabled. + + Expected result: + - Policies added are only in memory + - Reloading from database clears them + """ + mock_toggle.is_enabled.return_value = True + + enforcer = AuthzEnforcer.get_enforcer() + + test_policy = [ + make_role_key("test_role"), + make_action_key("test_action"), + make_scope_key("lib", "*"), + "allow", + ] + enforcer.add_policy(*test_policy) + + # Policy should be in memory + self.assertIn(test_policy, enforcer.get_policy()) + + # Reload from database - should clear memory-only policy + enforcer.load_policy() + policies = enforcer.get_policy() + + self.assertNotIn(test_policy, policies) + + @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") + @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0) + def test_multiple_get_enforcer_calls_preserve_auto_save(self, mock_toggle): + """Test that multiple get_enforcer() calls don't repeatedly disable auto-save. + + This is a regression test for the bug where get_enforcer() would + disable auto-save on every call when interval was 0. + + Expected result: + - After manually enabling auto-save, it stays enabled + - Multiple get_enforcer() calls don't change auto-save state + """ + mock_toggle.is_enabled.return_value = True + + # First call + enforcer1 = AuthzEnforcer.get_enforcer() + enforcer1.enable_auto_save(True) + self.assertTrue(enforcer1._e.auto_save) + + # Multiple subsequent calls + for _ in range(5): + enforcer = AuthzEnforcer.get_enforcer() + self.assertTrue(enforcer._e.auto_save) From cbb25ec496b6414cd768b5e681e5349415701999 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 29 Oct 2025 13:25:24 +0100 Subject: [PATCH 6/7] refactor: address quality issues --- openedx_authz/engine/enforcer.py | 13 ++++++++++++- openedx_authz/tests/test_enforcer.py | 28 ++++++++++++++-------------- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/openedx_authz/engine/enforcer.py b/openedx_authz/engine/enforcer.py index 112433d6..da32654f 100644 --- a/openedx_authz/engine/enforcer.py +++ b/openedx_authz/engine/enforcer.py @@ -82,6 +82,17 @@ def configure_enforcer_auto_loading(cls, auto_load_policy_interval: int = None): if not cls._enforcer.is_auto_loading_running(): cls._enforcer.start_auto_load_policy(auto_load_policy_interval) + @classmethod + def is_auto_save_enabled(cls) -> bool: + """Check if auto-save is currently enabled on the enforcer. + + Returns: + bool: True if auto-save is enabled, False otherwise + """ + if cls._enforcer is None: + return False + return cls._enforcer._e.auto_save # pylint: disable=protected-access + @classmethod def configure_enforcer_auto_save(cls, auto_save_policy: bool): """Configure auto-save on the enforcer. @@ -95,7 +106,7 @@ def configure_enforcer_auto_save(cls, auto_save_policy: bool): Returns: None """ - if cls._enforcer._e.auto_save != auto_save_policy: + if cls.is_auto_save_enabled() != auto_save_policy: cls._enforcer.enable_auto_save(auto_save_policy) @classmethod diff --git a/openedx_authz/tests/test_enforcer.py b/openedx_authz/tests/test_enforcer.py index 84c49f4c..ce72d735 100644 --- a/openedx_authz/tests/test_enforcer.py +++ b/openedx_authz/tests/test_enforcer.py @@ -6,7 +6,7 @@ """ import time -from unittest.mock import Mock, patch +from unittest.mock import patch import casbin from ddt import data as ddt_data @@ -594,7 +594,7 @@ def test_enforcer_auto_save_enabled_when_toggle_enabled(self, mock_toggle): enforcer = AuthzEnforcer.get_enforcer() - self.assertTrue(enforcer._e.auto_save) + self.assertTrue(AuthzEnforcer.is_auto_save_enabled()) test_policy = [ make_role_key("test_role"), @@ -623,7 +623,7 @@ def test_enforcer_deactivated_when_toggle_disabled(self, mock_toggle): enforcer = AuthzEnforcer.get_enforcer() - self.assertFalse(enforcer._e.auto_save) + self.assertFalse(AuthzEnforcer.is_auto_save_enabled()) self.assertFalse(enforcer.is_auto_loading_running()) @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") @@ -640,7 +640,7 @@ def test_enforcer_auto_load_starts_when_toggle_enabled(self, mock_toggle): enforcer = AuthzEnforcer.get_enforcer() self.assertTrue(enforcer.is_auto_loading_running()) - self.assertTrue(enforcer._e.auto_save) + self.assertTrue(AuthzEnforcer.is_auto_save_enabled()) @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0.5) @@ -660,7 +660,7 @@ def test_enforcer_auto_load_not_restarted_on_subsequent_calls(self, mock_toggle) enforcer2 = AuthzEnforcer.get_enforcer() self.assertIs(enforcer1, enforcer2) self.assertTrue(enforcer2.is_auto_loading_running()) - self.assertTrue(enforcer2._e.auto_save) + self.assertTrue(AuthzEnforcer.is_auto_save_enabled()) @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0) @@ -676,12 +676,12 @@ def test_toggle_state_checked_on_every_get_enforcer_call(self, mock_toggle): """ mock_toggle.is_enabled.return_value = False enforcer1 = AuthzEnforcer.get_enforcer() - self.assertFalse(enforcer1._e.auto_save) + self.assertFalse(AuthzEnforcer.is_auto_save_enabled()) mock_toggle.is_enabled.return_value = True enforcer2 = AuthzEnforcer.get_enforcer() self.assertIs(enforcer1, enforcer2) - self.assertTrue(enforcer2._e.auto_save) + self.assertTrue(AuthzEnforcer.is_auto_save_enabled()) @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0) @@ -701,7 +701,7 @@ def test_dummy_toggle_behavior_in_tests(self, mock_toggle): enforcer = AuthzEnforcer.get_enforcer() self.assertIsNotNone(enforcer) - self.assertTrue(enforcer._e.auto_save) + self.assertTrue(AuthzEnforcer.is_auto_save_enabled()) @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0) @@ -719,12 +719,12 @@ def test_auto_save_preserved_with_interval_zero(self, mock_toggle): enforcer = AuthzEnforcer.get_enforcer() enforcer.enable_auto_save(True) - self.assertTrue(enforcer._e.auto_save) + self.assertTrue(AuthzEnforcer.is_auto_save_enabled()) # Call get_enforcer() again - should not disable auto-save enforcer2 = AuthzEnforcer.get_enforcer() self.assertIs(enforcer, enforcer2) - self.assertTrue(enforcer2._e.auto_save) + self.assertTrue(AuthzEnforcer.is_auto_save_enabled()) @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=0) @@ -767,7 +767,7 @@ def test_auto_save_disabled_explicitly(self, mock_toggle): enforcer = AuthzEnforcer.get_enforcer() - self.assertFalse(enforcer._e.auto_save) + self.assertFalse(AuthzEnforcer.is_auto_save_enabled()) self.assertFalse(enforcer.is_auto_loading_running()) @patch("openedx_authz.engine.enforcer.libraries_v2_enabled") @@ -817,9 +817,9 @@ def test_multiple_get_enforcer_calls_preserve_auto_save(self, mock_toggle): # First call enforcer1 = AuthzEnforcer.get_enforcer() enforcer1.enable_auto_save(True) - self.assertTrue(enforcer1._e.auto_save) + self.assertTrue(AuthzEnforcer.is_auto_save_enabled()) # Multiple subsequent calls for _ in range(5): - enforcer = AuthzEnforcer.get_enforcer() - self.assertTrue(enforcer._e.auto_save) + AuthzEnforcer.get_enforcer() + self.assertTrue(AuthzEnforcer.is_auto_save_enabled()) From 315125db9b84bb20b9d448208998f22ee07bcc61 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 29 Oct 2025 16:40:16 +0100 Subject: [PATCH 7/7] docs: update docs to release changes --- CHANGELOG.rst | 8 ++++++++ openedx_authz/__init__.py | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 6f7047e9..591c5d92 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -16,6 +16,14 @@ Unreleased * +0.11.0 - 2025-10-29 +******************** + +Added +===== + +* Disable auto-save and auto-load of policies if Content Library V2 is disabled. + 0.10.1 - 2025-10-28 ******************** diff --git a/openedx_authz/__init__.py b/openedx_authz/__init__.py index fcdedfbc..ea0dd1d0 100644 --- a/openedx_authz/__init__.py +++ b/openedx_authz/__init__.py @@ -4,6 +4,6 @@ import os -__version__ = "0.10.1" +__version__ = "0.11.0" ROOT_DIRECTORY = os.path.dirname(os.path.abspath(__file__))