[FC-0099] refactor: use decorator to manage policy lifecycle for low-level APIs by mariajgrimaldi · Pull Request #86 · openedx/openedx-authz

mariajgrimaldi · 2025-10-07T19:13:09Z

Description

Implement a decorator to handle the policy lifecycle before and after calling any public API method. The goal is to free clients from manually loading and clearing the enforcer, ensuring each operation uses the latest policies from the datastore.

The decorator should:

Load the policy into the enforcer before calling the API method. This means reading the policy from the datastore (Django model) and loading it into memory, making it accessible to the enforcer instance.
Call the API function, which assumes it’s working with the latest policy data.
Clear the enforcer once the function completes, so it can be safely reused.

The main concern with this approach (and the previous one, though it's clearer here) is that we have a single enforcer per process. If two requests run in parallel and one finishes first, clearing the enforcer could affect the other request mid-operation.

Here's what I've been thinking:

A possible alternative is to create one enforcer per transaction, where a transaction represents a low-level API operation. The enforcer would be disposed of afterward, avoiding shared state between concurrent requests. Or instead of per transaction, per request.
Another option is to use a synced enforcer, which provides synchronized access but may reduce performance.
We make the enforcer a shared entity, read-only, loaded once at first and then reloaded (safely) only when there is a change here: https://github.com/openedx/openedx-authz/blob/main/openedx_authz/engine/watcher.py#L22-L32

What do you think?

Casbin references related to this topic:

openedx-webhooks · 2025-10-07T19:13:14Z

Thanks for the pull request, @mariajgrimaldi!

This repository is currently maintained by @openedx/committers-openedx-authz.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

mariajgrimaldi · 2025-10-08T12:50:49Z

@@ -0,0 +1,377 @@
+"""Test cases for decorator functions.


TODO: need to add tests for action grouping at least for enforcement.

…nt states

bmtcril · 2025-10-14T15:02:08Z

+from openedx_authz.engine.filter import Filter
+
+
+def manage_policy_lifecycle(filter_on: str = ""):


nit: You can probably just annotate this return as Any from typing.

bmtcril · 2025-10-14T16:52:53Z

+            Filter: A Filter object populated with relevant filter values.
+        """
+        # Fallback to no filtering in case of misbehavior
+        if settings.ALLOW_FILTERED_POLICY_LOADING is False:


Are we planning on getting this working for Ulmo? If not, I would rather temporarily remove the code we're not using than have code that we don't trust just disabled via a flag.

I agree! I can remove it and save it in another branch. My initial idea was to start by loading the entire policy and then address any issues with this approach retroactively. I did try adding support for filtered loading, but I ran into some issues while finishing the implementation and adding tests. As I mentioned here: https://github.com/openedx/openedx-authz/pull/86/files#diff-a6773904192669f0814472909dcec3567d5a893f6a97e6b8682cdd2ba8bcb55dR40-R55, in my opinion, making filtered loading consistent would probably require a different model, which isn't a priority right now. We could revisit it later if we notice resource consumption issues.

BryanttV

It looks great, I just have a few comments. I'll be testing these changes more thoroughly in my local environment.

BryanttV · 2025-10-14T16:52:09Z


    NAMESPACE: ClassVar[str] = "sc"
+    POLICY_POSITION = 2  # Position of scope in Casbin policy rules (p = sub, act, obj)
+    GROUPING_POLICY_POSITION = 2  # Position of scope in Casbin grouping policy rules (g = sub, role, scope)


Unused variable

BryanttV · 2025-10-14T16:57:37Z

+        Returns:
+            str: The policy template string.
+        """
+        return f"{self.NAMESPACE}{self.SEPARATOR}*"


Could we use the GENERIC_SCOPE_WILDCARD variable instead of the * literal character?

BryanttV · 2025-10-14T17:00:03Z

        raise NotImplementedError("Subclasses must implement exists method.")

+    @property
+    def policy_template(self) -> str:


Could we add examples in the docstring? I think they are very useful for understanding.

BryanttV · 2025-10-14T17:23:33Z

+        # Without filtering (loads all policies):
+        @manage_policy_lifecycle()
+        def get_all_roles():
+            return enforcer.get_all_roles()
+
+        # With scope filtering (loads only relevant policies):
+        @manage_policy_lifecycle(filter_on="scope")
+        def get_roles_in_scope(scope: ScopeData):
+            return enforcer.get_filtered_roles(scope.namespaced_key)


To highlight the code:

Suggested change

# Without filtering (loads all policies):

@manage_policy_lifecycle()

def get_all_roles():

return enforcer.get_all_roles()

# With scope filtering (loads only relevant policies):

@manage_policy_lifecycle(filter_on="scope")

def get_roles_in_scope(scope: ScopeData):

return enforcer.get_filtered_roles(scope.namespaced_key)

- Without filtering (loads all policies)::

@manage_policy_lifecycle()

def get_all_roles():

return enforcer.get_all_roles()

- With scope filtering (loads only relevant policies)::

@manage_policy_lifecycle(filter_on="scope")

def get_roles_in_scope(scope: ScopeData):

return enforcer.get_filtered_roles(scope.namespaced_key)

BryanttV · 2025-10-14T17:25:43Z



+@manage_policy_lifecycle(filter_on="scope")
 def is_subject_allowed(


Please, remove the enforcer.load_policy()

BryanttV · 2025-10-14T17:29:17Z

+@manage_policy_lifecycle()
 def get_permissions_for_single_role(


I'm concerned about having to load the policy twice, since this function is called internally by other functions here.

BryanttV

I have already tested the changes using the REST API with different users, and it loads the policies correctly. Thanks!

BryanttV · 2025-10-14T18:02:11Z

    return [get_permission_from_policy(action) for action in actions]


+@manage_policy_lifecycle(filter_on="scope")


Suggested change

@manage_policy_lifecycle(filter_on="scope")

@manage_policy_lifecycle()

BryanttV · 2025-10-14T18:03:02Z

Please add the decorator to the get_subjects_for_role function

mariajgrimaldi · 2025-10-15T10:42:18Z

Thanks for the review, @bmtcril @BryanttV! I'll be going through your comments later today :)

What do you think of the concern I documented in the cover letter? Let me know!

bmtcril · 2025-10-15T17:18:36Z

Thanks for the review, @bmtcril @BryanttV! I'll be going through your comments later today :)

What do you think of the concern I documented in the cover letter? Let me know!

Here are my assumptions, please correct me if I'm wrong:

The policy is a combination all of the permissions, roles, permission inheritance, and role assignments to users
The policy can be partially invalidated by adding or deleting rows (at runtime this should always be granting or revoking roles from users, I think?)

I think I've come around to the sync'd enforcer with a short timeout of like 5-10 seconds for the following reasons:

Why not option 1: Pulling all rows from MySQL once or several times per view can be a very heavy operation on large instances
Why not option 3: redis-watcher doesn't support reconnection, so if redis goes down or the application loses the connection the application will be stuck with an old policy until restart (see here and here)
I'd considered a pull through redis cache, but I think the whole policy is likely to grow too large to store in redis.

Compared to these, a few seconds of staleness seems to be the lesser evil. It is worth noting that it would still be 1 enforcer per process (by default Tutor runs 2 processes per container) so on a large instance with, say 15 lms/cms containers and 5 lms-worker/cms-worker containers running (each is only 1 process). So with 35 distinct Python processes it would still constantly hit the database several times a second at a 10 second cadence (though in most cases it would probably be cached). As such we should definitely make this configurable if we go this way.

Thoughts for the future:

If the watcher could handle service interruptions better I think that would be the way to go
I'd be curious to see how much we can scope our access patterns down to smaller chunks and aggressively cache those in redis. I'd expect that by far the majority of interactions with the policy would be scoped permissions checks for a single user, which should be cacheable. Combining this with a more resilient watcher might be our best option.

BryanttV · 2025-10-20T21:24:18Z

Hi @bmtcril, thanks for your proposal! I just created a PR with the necessary changes to use the SynEnforcer here: #103, Any questions or suggestions are welcome!

mariajgrimaldi · 2025-10-22T08:17:30Z

If approved, this PR should be closed in favor of #103

bmtcril · 2025-10-22T12:47:58Z

If approved, this PR should be closed in favor of #103

@mariajgrimaldi I approved on that one, but would appreciate your review there too

mariajgrimaldi · 2025-10-23T09:03:57Z

This PR will be closed in favor of #103, other efforts to use load_filtered_policy will be addressed as part of a different effort which could reuse this initial approach.

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core contributor PR author is a Core Contributor (who may or may not have write access to this repo). labels Oct 7, 2025

openedx-webhooks added this to Contributions Oct 7, 2025

github-project-automation Bot moved this to Needs Triage in Contributions Oct 7, 2025

mariajgrimaldi force-pushed the MJG/policy-decorator branch 3 times, most recently from e2e50de to e1ccc1b Compare October 7, 2025 19:19

mariajgrimaldi mentioned this pull request Oct 7, 2025

[FC-0099] feat: add rest api for roles and permissions #84

Merged

mariajgrimaldi commented Oct 8, 2025

View reviewed changes

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Oct 8, 2025

mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Oct 8, 2025

Base automatically changed from MJG/public-api to main October 10, 2025 09:38

mariajgrimaldi force-pushed the MJG/policy-decorator branch from e1ccc1b to 09f5f79 Compare October 10, 2025 10:04

mariajgrimaldi added 7 commits October 14, 2025 11:13

feat: add decorator to manage policy lifecycle for low-level APIs

b0de005

refactor: remove level of nested

33a14f4

refactor: consider no filtering as first version

c27328e

refactor: address quality issues

4d0cfb5

refactor: address quality issues

78cdd84

docs: update inline doc with better structure

d666e35

docs: address quality issues

3dca2a2

mariajgrimaldi force-pushed the MJG/policy-decorator branch from 167f4fd to 3dca2a2 Compare October 14, 2025 09:18

refactor: drop filter_on in this first version

1f6d978

mariajgrimaldi changed the title ~~[FC-0099] feat: add decorator to manage policy lifecycle for low-level APIs~~ [FC-0099] refactor: use decorator to manage policy lifecycle for low-level APIs Oct 14, 2025

mariajgrimaldi requested review from BryanttV, MaferMazu, bmtcril and rodmgwgu October 14, 2025 11:50

refactor: avoid clearing policies after finishing to avoid inconsiste…

9223611

…nt states

bmtcril reviewed Oct 14, 2025

View reviewed changes

BryanttV suggested changes Oct 14, 2025

View reviewed changes

BryanttV reviewed Oct 14, 2025

View reviewed changes

BryanttV mentioned this pull request Oct 20, 2025

[FC-0099] feat: use synced enforcer #103

Merged

7 tasks

MaferMazu added this to RBAC AuthZ Board Oct 22, 2025

mariajgrimaldi moved this to Blocked in RBAC AuthZ Board Oct 22, 2025

mariajgrimaldi closed this Oct 23, 2025

github-project-automation Bot moved this from Blocked to Done in RBAC AuthZ Board Oct 23, 2025

github-project-automation Bot moved this from Waiting on Author to Done in Contributions Oct 23, 2025

mariajgrimaldi mentioned this pull request Nov 4, 2025

Hard-load of Casbin policy on permission validation #133

Closed

mariajgrimaldi mentioned this pull request Dec 5, 2025

Implement a scalable policy loading strategy #158

Open

4 tasks

		from openedx_authz.engine.filter import Filter


		def manage_policy_lifecycle(filter_on: str = ""):



		@manage_policy_lifecycle(filter_on="scope")
		def is_subject_allowed(

		@manage_policy_lifecycle()
		def get_permissions_for_single_role(

		return [get_permission_from_policy(action) for action in actions]


		@manage_policy_lifecycle(filter_on="scope")

Conversation

mariajgrimaldi commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

openedx-webhooks commented Oct 7, 2025

Uh oh!

mariajgrimaldi Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariajgrimaldi Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanttV left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanttV Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanttV left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariajgrimaldi commented Oct 15, 2025

Uh oh!

bmtcril commented Oct 15, 2025

Uh oh!

BryanttV commented Oct 20, 2025

Uh oh!

mariajgrimaldi commented Oct 22, 2025

Uh oh!

bmtcril commented Oct 22, 2025

Uh oh!

mariajgrimaldi commented Oct 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mariajgrimaldi commented Oct 7, 2025 •

edited

Loading

mariajgrimaldi Oct 8, 2025 •

edited

Loading

mariajgrimaldi Oct 14, 2025 •

edited

Loading

BryanttV Oct 14, 2025 •

edited

Loading