From 2c7d8434d6c76036d8a0814eb1817ef77f4dfec8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Apr 2026 23:31:25 +0000 Subject: [PATCH 1/2] perf: add per-request RequestCache to is_user_allowed Agent-Logs-Url: https://github.com/openedx/openedx-authz/sessions/c95aaea6-7134-4d75-bb96-f9ab7e1eda4d Co-authored-by: MaferMazu <35668326+MaferMazu@users.noreply.github.com> --- CHANGELOG.rst | 12 ++++++ openedx_authz/__init__.py | 2 +- openedx_authz/api/users.py | 26 ++++++++++++- openedx_authz/tests/api/test_users.py | 55 +++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 82e50821..c0ec8dcc 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -14,6 +14,18 @@ Change Log Unreleased ********** +1.15.0 - 2026-04-24 +******************* + +Performance +=========== + +* Add a per-request ``RequestCache`` to ``is_user_allowed`` to prevent redundant Casbin + enforcement calls when the same ``(user, action, scope)`` triple is checked multiple times + within a single HTTP request (e.g., once per object-tag during serialization). + The cache is automatically cleared whenever a role assignment changes via the user API + so that permission checks within the same request always reflect the current state. + 1.14.0 - 2026-04-22 ******************* diff --git a/openedx_authz/__init__.py b/openedx_authz/__init__.py index 28e97ad9..c55ab814 100644 --- a/openedx_authz/__init__.py +++ b/openedx_authz/__init__.py @@ -4,6 +4,6 @@ import os -__version__ = "1.14.0" +__version__ = "1.15.0" ROOT_DIRECTORY = os.path.dirname(os.path.abspath(__file__)) diff --git a/openedx_authz/api/users.py b/openedx_authz/api/users.py index e84bd590..e772690e 100644 --- a/openedx_authz/api/users.py +++ b/openedx_authz/api/users.py @@ -11,6 +11,7 @@ from django.contrib.auth import get_user_model from django.db.models import Q +from edx_django_utils.cache import RequestCache from openedx_authz.api.data import ( ActionData, @@ -43,6 +44,10 @@ User = get_user_model() +# Cache namespace used by is_user_allowed. Cleared by role mutation functions so +# that permission checks within the same request reflect the latest assignments. +_IS_USER_ALLOWED_CACHE_NS = "rbac_is_user_allowed" + __all__ = [ "assign_role_to_user_in_scope", @@ -76,6 +81,7 @@ def assign_role_to_user_in_scope(user_external_key: str, role_external_key: str, Returns: bool: True if the role was assigned successfully, False otherwise. """ + RequestCache(_IS_USER_ALLOWED_CACHE_NS).clear() return assign_role_to_subject_in_scope( UserData(external_key=user_external_key), RoleData(external_key=role_external_key), @@ -91,6 +97,7 @@ def batch_assign_role_to_users_in_scope(users: list[str], role_external_key: str role_external_key (str): Name of the role to assign. scope (str): Scope in which to assign the role. """ + RequestCache(_IS_USER_ALLOWED_CACHE_NS).clear() namespaced_users = [UserData(external_key=username) for username in users] batch_assign_role_to_subjects_in_scope( namespaced_users, @@ -110,6 +117,7 @@ def unassign_role_from_user(user_external_key: str, role_external_key: str, scop Returns: bool: True if the role was unassigned successfully, False otherwise. """ + RequestCache(_IS_USER_ALLOWED_CACHE_NS).clear() return unassign_role_from_subject_in_scope( UserData(external_key=user_external_key), RoleData(external_key=role_external_key), @@ -125,6 +133,7 @@ def batch_unassign_role_from_users(users: list[str], role_external_key: str, sco role_external_key (str): Name of the role to unassign. scope (str): Scope in which to unassign the role. """ + RequestCache(_IS_USER_ALLOWED_CACHE_NS).clear() namespaced_users = [UserData(external_key=user) for user in users] batch_unassign_role_from_subjects_in_scope( namespaced_users, @@ -345,6 +354,11 @@ def is_user_allowed( ) -> bool: """Check if a user has a specific permission in a given scope. + Results are cached per-request (keyed by user, action, and scope) to avoid + repeated enforcement calls for the same arguments within a single request, + e.g. when permission checks are performed once per object-tag during + serialization. + Args: user_external_key (str): ID of the user (e.g., 'john_doe'). action_external_key (str): The action to check (e.g., 'view_course'). @@ -353,11 +367,20 @@ def is_user_allowed( Returns: bool: True if the user has the specified permission in the scope, False otherwise. """ - return is_subject_allowed( + request_cache = RequestCache(_IS_USER_ALLOWED_CACHE_NS) + cache_key = f"{user_external_key}:{action_external_key}:{scope_external_key}" + + cached_response = request_cache.get_cached_response(cache_key) + if cached_response.is_found: + return cached_response.value + + result = is_subject_allowed( UserData(external_key=user_external_key), ActionData(external_key=action_external_key), ScopeData(external_key=scope_external_key), ) + request_cache.set(cache_key, result) + return result def get_users_for_role_in_scope(role_external_key: str, scope_external_key: str) -> list[UserData]: @@ -405,6 +428,7 @@ def unassign_all_roles_from_user(user_external_key: str) -> bool: Returns: bool: True if any roles were removed, False otherwise. """ + RequestCache(_IS_USER_ALLOWED_CACHE_NS).clear() return unassign_subject_from_all_roles(UserData(external_key=user_external_key)) diff --git a/openedx_authz/tests/api/test_users.py b/openedx_authz/tests/api/test_users.py index da0b3b75..96e19ed1 100644 --- a/openedx_authz/tests/api/test_users.py +++ b/openedx_authz/tests/api/test_users.py @@ -4,6 +4,7 @@ from ddt import data, ddt, unpack from django.contrib.auth import get_user_model +from edx_django_utils.cache import RequestCache from openedx_authz.api.data import ContentLibraryData, RoleAssignmentData, RoleData, UserData from openedx_authz.api.users import ( @@ -521,6 +522,60 @@ def test_is_user_allowed(self, username, action, scope_name, expected_result): self.assertEqual(result, expected_result) +class TestIsUserAllowedRequestCache(UserAssignmentsSetupMixin): + """Test that is_user_allowed uses a per-request cache to avoid redundant enforcement calls.""" + + def setUp(self): + super().setUp() + # Clear the request cache before each test so results don't bleed across tests. + RequestCache.clear_all_namespaces() + + def test_cache_hit_on_repeated_call(self): + """Repeated calls with identical arguments should only invoke the enforcer once.""" + username = "alice" + action = permissions.DELETE_LIBRARY.identifier + scope = "lib:Org1:math_101" + + with patch("openedx_authz.api.users.is_subject_allowed") as mock_enforce: + mock_enforce.return_value = True + first = is_user_allowed(user_external_key=username, action_external_key=action, scope_external_key=scope) + second = is_user_allowed(user_external_key=username, action_external_key=action, scope_external_key=scope) + + self.assertTrue(first) + self.assertEqual(first, second) + # The underlying enforcement should have been invoked only once; the second call is served from cache. + self.assertEqual(mock_enforce.call_count, 1) + + def test_cache_miss_on_different_scope(self): + """Different scope arguments must produce independent cache entries.""" + username = "alice" + action = permissions.DELETE_LIBRARY.identifier + + with patch("openedx_authz.api.users.is_subject_allowed") as mock_enforce: + mock_enforce.return_value = True + is_user_allowed(user_external_key=username, action_external_key=action, scope_external_key="lib:Org1:math_101") + is_user_allowed(user_external_key=username, action_external_key=action, scope_external_key="lib:Org1:math_advanced") + + # Each distinct scope key is a separate cache entry → two enforcer calls. + self.assertEqual(mock_enforce.call_count, 2) + + def test_cache_cleared_after_role_mutation(self): + """The cache must be invalidated when a role assignment changes within the same request.""" + username = "alice" + action = permissions.DELETE_LIBRARY.identifier + scope = "lib:Org1:math_101" + + # alice has the permission before unassignment + before = is_user_allowed(user_external_key=username, action_external_key=action, scope_external_key=scope) + self.assertTrue(before) + + # Unassigning roles should clear the cache so the next call goes to the enforcer. + unassign_all_roles_from_user(user_external_key=username) + + after = is_user_allowed(user_external_key=username, action_external_key=action, scope_external_key=scope) + self.assertFalse(after) + + @ddt class TestValidateUsersAPI(UserAssignmentsSetupMixin): """Test suite for validate_users API function - focused on business logic.""" From 533e096a659472cb2a26cc0fd3278b54516787ac Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Apr 2026 23:43:15 +0000 Subject: [PATCH 2/2] fix: shorten long lines in test_users.py to satisfy pylint line-too-long Agent-Logs-Url: https://github.com/openedx/openedx-authz/sessions/51fd84d4-f9ee-4d18-9bef-b451810408a6 Co-authored-by: MaferMazu <35668326+MaferMazu@users.noreply.github.com> --- openedx_authz/tests/api/test_users.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/openedx_authz/tests/api/test_users.py b/openedx_authz/tests/api/test_users.py index 96e19ed1..abebc31f 100644 --- a/openedx_authz/tests/api/test_users.py +++ b/openedx_authz/tests/api/test_users.py @@ -553,8 +553,12 @@ def test_cache_miss_on_different_scope(self): with patch("openedx_authz.api.users.is_subject_allowed") as mock_enforce: mock_enforce.return_value = True - is_user_allowed(user_external_key=username, action_external_key=action, scope_external_key="lib:Org1:math_101") - is_user_allowed(user_external_key=username, action_external_key=action, scope_external_key="lib:Org1:math_advanced") + is_user_allowed( + user_external_key=username, action_external_key=action, scope_external_key="lib:Org1:math_101" + ) + is_user_allowed( + user_external_key=username, action_external_key=action, scope_external_key="lib:Org1:math_advanced" + ) # Each distinct scope key is a separate cache entry → two enforcer calls. self.assertEqual(mock_enforce.call_count, 2)