[FC-0099] feat: add casbin-based authorization engine

[BryanttV]

Description

This PR introduces the Casbin engine integration for Open edX. It includes:

• Enforcer: Main entry point using FastEnforcer with DB adapter and Redis watcher for distributed policy evaluation.
• Adapter: Extended Django ORM adapter with filtering for efficient and selective policy loading.
• Watcher: Redis-based synchronization to propagate policy changes across instances.

Additional Components

• Filter: Utility class to load specific subsets of policies by type, subject, action, or scope.

Supporting Information

• Open edX AuthZ Framework Long Term Vision

[openedx-webhooks]

Thanks for the pull request, @BryanttV!

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:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

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]

[critical] Do we have to do this each time we load a filtered policy? Can we do it once during initialization?

[mariajgrimaldi]

[non-critical] Why are we setting enable_log=False?

[BryanttV]

Logging is supposed to be disabled by default, so we don't need to do it explicitly.

[mariajgrimaldi]

[non-critical] Not sure I follow, what don't we need to do explicitly?

[BryanttV]

If we have logging enabled, each time we perform an enforce a log with the result will be shown. https://casbin.org/docs/log-error/#logging. However, it’s disabled by default, so there’s no need to pass the argument (unless we want to enable it)

[mariajgrimaldi]

It makes sense to have it on, no?

[BryanttV]

Yes, although I’m not sure how much noise it could cause once there are many enforcements. But we could enable it.

[bmtcril]

Having a log of enforcement actions is one of the requirements. I'd rather have it in a separate file than the tracking logs if possible due to volume, though, which it looks like it supports.

[mariajgrimaldi]

[critical] Why is this line needed? Considering that we already have a filtered queryset to work on, do we need to evaluate it here? I'm guessing there's something I'm missing about casbin internals here. Let me know!

[BryanttV]

Yes, because we need the policy to be loaded in a format that Casbin can work with. The load_filtered_policy method doesn’t return anything. It just performs a filtered query and, based on that query, persists each line into the policy list.

It’s done the same way in the SQLAlchemy adapter

[mariajgrimaldi]

Right. This will cause a problem when trying to associate additional data to the Casbin rule, like metadata, because we will lose the relationship between this model's records and any other when loading the line. If we keep it as is, that is. Additionally, depending on the level of granularity in the filter, it may also be considered a performance concern.

Do you have any suggestions on how we can address this issue? I'll try to think about something.

⚠️ This is not a problem or a blocker for this particular PR but it is for our approach for adding additional metadata to casbin rules.

[mariajgrimaldi]

Maybe we could link the rule itself to our extended policy model so the lookup could be O(1) since each rule is unique, we can go over it when we open the PR for the model.

[BryanttV]

Just to clarify. The load_filtered_policy doesn’t modify anything in the database, so the IDs of each rule in the Casbin table will remain the same. What it does during the load is keep the policies in memory based on the filter, so they can later be used in other Casbin methods like get_roles_for_user, get_users_for_role, etc.

The problem might lie in how we’re going to access the metadata, since we wouldn’t know the ID of each loaded policy.

What you’re suggesting could be a solution, we could give it a try.

[mariajgrimaldi]

@BryanttV: exatcly, our main problem would be losing the linking between the two models.

@MaferMazu: here's some of the ideas we've discussed today about storing metadata as an extended policy model.

[mariajgrimaldi]

[critical] I'm thinking about where we should call this and how often, and I have a few ideas. Let me know what you think:

1. We originally said we shouldn't hit the database on every enforce, but this might still be acceptable for the MVP.
2. We could call this from middleware or at the view level, where the filter is: the requested org, user, object, or SAOC request. This depends on what's available. I think this could cover all enforcement queries, including explicit calls to our views or those using the has_permission helper we're adding in api/*.

These questions lead me to another point about cache management and its relation to the watcher. What does loading a subset of policies mean in this context? To me, it means loading them into Redis as part of cache management, with the watcher invalidating entries when the loaded policy (filtered or not) changes. Am I understanding this correctly?

Here's a diagram of what I think this should work: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/5210112002/Open+edX+AuthZ+Framework+Long-Term+Vision#Policy-Loading-Strategy

[mariajgrimaldi]

I'm working on the public API and facing the same question at a lower level: where should the filtered policy be loaded? Right now, I'm treating all calls as scoped. That means I could call load_filtered_policy each time a function runs with the given scope, then work with the resulting rules. For testing, I'm not yet loading filtered policies.

In any case, looking at the bigger picture, I think this approach might work:

• Receive a request with a scope, defaulting to global if none is provided
• Load the filtered policy into the adapter (still not sure if cache should be managed here, as mentioned before)
• If the policy changes, clear the enforcer and reload it (not sure "clear" is the best term here)
• If a new request comes in with a different scope, load that new scope

Do you think this makes sense?

[BryanttV]

@mariajgrimaldi, For the MVP, I agree it might be acceptable to hit the database more frequently. Also, I agree with your last approach, we can call load_filtered_policy at the view level (or middleware), based on the request scope (org, course, user, etc.). This way, we keep the logic centralized and consistent.

I’d like to expand on two points you brought up:

1. The watcher only notifies enforcers when a policy change happens, but the actual synchronization should happen in the callback. Right now, we only have logging there, but in most setups, you’d see something like enforcer.load_policy(). The issue is that we don’t want to reload the entire policy into memory every time, since that could be too heavy.
2. About the FastEnforcer cache, I’m still digging into how the caching mechanism works. At the moment, since we haven’t configured any cache (i.e., nothing is passed in the cache_key_order argument), it’s falling back to the standard enforcer of Casbin.

[mariajgrimaldi]

Opened issues to address this:
#73
#72

[mariajgrimaldi]

[critical] So if we use cache for the policies as I mentioned here, we could invalidate them here?

[mariajgrimaldi]

Suggested change

	# Redis host and port are temporarily loaded here for the MVP
	# TODO: Replace with a more dynamic configuration... Redis host and port are temporarily loaded here for the MVP

[critical] Also can we load it from the CACHE configuration? Some people might change the redis host for their installation, but if they do, they should also change that setting - that's the setting I know, not sure if there's a different one to configure redis.

[BryanttV]

I updated the comment to include your suggestion.

I checked that setting, but I'm not sure it will work for us. I think we can leave it as it is for now until more general connection variables are defined, as @bmtcril mentioned.

[mariajgrimaldi]

Please let's address this here: #73, if we're going to use redis we might need to solve this somehow

[mariajgrimaldi]

I think my review is done for now. Sorry it wasn't continuous. If you address the critical comments and also check the non-critical ones I labeled, we can move forward with the rest of the PRs. Thanks a lot for this, it's looking good!

[bmtcril]

We've recently run into cases where people are setting passwords and/or usersnames on redis as well, so we may need a more general set of connection variables

[bmtcril]

I've been through this and done some local testing on the branch, I agree with @mariajgrimaldi that we can move forward with this after the critical suggestions are added and tests pass. Thanks for all of the hard work on this!

[BryanttV]

Hi everyone, the PR has been updated with the latest changes. The idea is to address the documentation and coverage checks in another PR.
@mariajgrimaldi @bmtcril

[mariajgrimaldi]

I've been using this implementation while working on the public API for the framework, and it's working well. Thanks a lot!

I'd suggest not focusing on the coverage or docs failures right now. Instead, let's open new issues so we can handle those later, since we have more immediate issues to address. Thanks!