[DO NOT MERGE] feat: basic casbin tests#37
Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @BryanttV! This repository is currently maintained by 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 approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
| print("\n\nUpdate for remove filtered policy callback, event: {}".format(event)) | ||
|
|
||
|
|
||
| def get_enforcer(): |
There was a problem hiding this comment.
I have a few comments about this approach:
- Can we create a package called engine to move these kinds of definitions there? We could use it for all definitions related to our authorization engine.
- Can we implement a class instead, called
Enforcer(or another most distinguished name) that behaves like this enforcer but sets up the watcher as part of its initialization (ducktyping)? We could also implement more customizations if needed. - Do we have to initialize the watcher manually here everytime we get an enforcer? Can't we use CASBIN_WATCHER setting instead or is it configured per enforcer-instance?
- Do we need an enforcer per django process?
There was a problem hiding this comment.
- Yes, I think that’s a good idea. Centralizing all those definitions we’ll need later seems like the most appropriate approach.
- Yes, I agree. We should have a class that wraps the original Enforcer. That way we could initialize the Watcher, Adapter, or anything else, or add any custom methods we need, such as
load_filtered_policy. - I wasn’t aware of the
CASBIN_WATCHERsetting. It could be useful to avoid the issue you mentioned. I’ll be running some tests with that configuration. - Casbin doesn’t share state across processes, so a watcher would be needed to synchronize changes. Is that what you’re referring to?
There was a problem hiding this comment.
I wasn’t aware of the CASBIN_WATCHER setting. It could be useful to avoid the issue you mentioned. I’ll be running some tests with that configuration
See #37 (comment)
| settings.INSTALLED_APPS.append(app) | ||
|
|
||
| # Add middleware for authorization | ||
| middleware_class = "dauthz.middlewares.request_middleware.RequestMiddleware" |
There was a problem hiding this comment.
This request middleware loads the entire policy into memory using enforcer.load_policy: https://github.com/pycasbin/django-authorization/blob/83a70ddcc2cd53c152b6de5f31af974c658918f9/dauthz/middlewares/request_middleware.py#L7-L15 which could severely impact performance in real production settings. I don't think using this middleware as is is viable for our use case.
This also takes me to this question here: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/5210112002/WIP+Open+edX+AuthZ+Framework+Long-Term+Vision?focusedCommentId=5220433941. Django ORM adapter which is used by this library doesn't support loading filtered policies, which again is not a viable option for us loading an entire database into memory. What I think we could do is implementing our own extended adapter which filters policies, using as guide what's already implemented in the adapter itself and also in other python adapters which do support filtering policies like https://github.com/officialpycasbin/sqlalchemy-adapter
We could do something like (pseudocode, haven't tested it):
from casbin_adapter.adapter import Adapter
from casbin_adapter.models import CasbinRule
class ExtendedAdapter(Adapter):
def __init__(self):
super().__init__()
def load_filtered_policy(self, filter):
"""Load only policy rules that match the filter.
This filter should come from a more human-readable query format, e.g.:
{
"ptype": "p",
"rule": ["alice", "data1", "read"]
}
"""
query_params = {"ptype": filter.get("ptype")}
for i, v in enumerate(filter.get("rule", [])):
query_params["v{}".format(i)] = v
return CasbinRule.objects.using(self.db_alias).filter(**query_params).all()
There was a problem hiding this comment.
I tested this in the django shell, it seems to work:
In [3]: enforcer.adapter
Out[3]: <openedx_authz.engine.adapter.ExtendedAdapter at 0x71ca1e7e18d0>
In [4]: enforcer.adapter.load_filtered_policy({"ptype": "p", "rule": ['anonymous', '/login', '*']})
Out[4]: <QuerySet [<CasbinRule 2: "p, anonymous, /login, *">]>
The filter will probably have to be more complex and the query used should also consider other use cases like loading for a single org etc which I haven't tested. At least I think this could work.
There is still a concern about the performance implications of these queries, though.
There was a problem hiding this comment.
Thanks for your comment!
Yes, based on what you mentioned, we shouldn’t use the default Middleware. I also tested your implementation, but I wonder if we should do something additional. You’re calling enforcer.adapter.load_filtered_policy, but we should be able to use the enforcer directly like enforcer.load_filtered_policy.
I’m also noticing that if we don’t use the middleware, the django-authorization library isn’t necessary. I’m running some tests using only the Django ORM Adapter.
There was a problem hiding this comment.
Based on your code and the SQLAlchemy adapter, I created this ExtendedAdapter (It may need some optimization, but the adapter seems to work correctly with the new method.)
# openedx_authz.engine.adapter
from casbin import persist
from casbin.persist import FilteredAdapter
from casbin_adapter.adapter import Adapter
from casbin_adapter.models import CasbinRule
class ExtendedAdapter(Adapter, FilteredAdapter):
"""
Extended adapter for the casbin model.
"""
_filtered = False
def load_filtered_policy(self, model, filter) -> None: # pylint: disable=redefined-builtin
"""loads all policy rules from the storage."""
queryset = CasbinRule.objects.using(self.db_alias).all()
filtered_queryset = self.filter_query(queryset, filter)
for line in filtered_queryset:
persist.load_policy_line(str(line), model)
self._filtered = True
def filter_query(self, queryset, filter): # pylint: disable=redefined-builtin
"""filters the queryset based on the attributes of the filter."""
for attr in ("ptype", "v0", "v1", "v2", "v3", "v4", "v5"):
filter_values = getattr(filter, attr)
if len(filter_values) > 0:
filter_kwargs = {f"{attr}__in": filter_values}
queryset = queryset.filter(**filter_kwargs)
return queryset.order_by("id")# openedx_authz.engine.filter
class Filter:
"""
Filter class for the casbin model.
"""
ptype = []
v0 = []
v1 = []
v2 = []
v3 = []
v4 = []
v5 = []I did some tests, and it seems to work:
In [1]: from openedx_authz.engine.filter import Filter
In [2]: from casbin_adapter.enforcer import enforcer as e
In [3]: f = Filter()
In [4]: f.v0 = ["alice"]
In [5]: e.load_filtered_policy(f)
2025-09-10 22:52:36,830 INFO 147 [casbin.policy] [user None] [ip None] policy.py:73 - Policy:
2025-09-10 22:52:36,830 INFO 147 [casbin.policy] [user None] [ip None] policy.py:79 - p : sub, obj, act : [['alice', 'data1', 'read']]
2025-09-10 22:52:36,831 INFO 147 [casbin.policy] [user None] [ip None] policy.py:79 - g : _, _ : [['alice', 'data2_admin']]
2025-09-10 22:52:36,831 INFO 147 [casbin.policy] [user None] [ip None] assertion.py:48 - Role links for: g
2025-09-10 22:52:36,831 INFO 147 [casbin.role] [user None] [ip None] role_manager.py:218 - alice < data2_admin
In [6]: e.get_policy()
Out[6]: [['alice', 'data1', 'read']]
In [7]: f.v0 = ["bob"]
In [8]: e.load_filtered_policy(f)
2025-09-10 22:52:48,271 INFO 147 [casbin.policy] [user None] [ip None] policy.py:73 - Policy:
2025-09-10 22:52:48,272 INFO 147 [casbin.policy] [user None] [ip None] policy.py:79 - p : sub, obj, act : [['bob', 'data2', 'write']]
2025-09-10 22:52:48,272 INFO 147 [casbin.policy] [user None] [ip None] policy.py:79 - g : _, _ : []
2025-09-10 22:52:48,272 INFO 147 [casbin.policy] [user None] [ip None] assertion.py:48 - Role links for: g
2025-09-10 22:52:48,272 INFO 147 [casbin.role] [user None] [ip None] role_manager.py:218 -
In [9]: e.get_policy()
Out[9]: [['bob', 'data2', 'write']]There was a problem hiding this comment.
This is the new Casbin config using only the Django ORM Adapter:
casbin_adapter_app = "casbin_adapter.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, "model.conf")
watcher_options = WatcherOptions()
watcher_options.host = "redis"
watcher_options.port = 6379
watcher_options.optional_update_callback = callback_function
watcher = new_watcher(watcher_options)
settings.CASBIN_WATCHER = watcher
settings.CASBIN_ADAPTER = "openedx_authz.engine.adapter.ExtendedAdapter"There was a problem hiding this comment.
Great! Where can we get the redis configurations though? I see it's in the CACHE dictionary or maybe this could be part of a casbin.py on each installation - which is not tied directly to lms/cms
| settings.MIDDLEWARE = settings.MIDDLEWARE + [middleware_class] | ||
|
|
||
| # Add authorization configuration | ||
| settings.CASBIN_MODEL = os.path.join(ROOT_DIRECTORY, "model.conf") |
There was a problem hiding this comment.
This should be a file part of a tutor plugin instead so it can be loaded and accessed as a volume and properly maintained by operators.
| from .models import Library | ||
| from .serializers import LibrarySerializer | ||
|
|
||
| enforcer = get_enforcer() |
There was a problem hiding this comment.
Echo to my previous question: do we need an instance of the enforcer here? Does this mean, 1 instance per django process?
There was a problem hiding this comment.
In my last tests importing from casbin_adapter.enforcer import enforcer we can use it with the custom Casbin configuration in the settings.py
| enforcer.add_policy( | ||
| self.request.user.username, | ||
| f"{self.request.path}{library.id}/", | ||
| "(GET)|(PUT)|(DELETE)|(PATCH)", | ||
| ) |
There was a problem hiding this comment.
I think these kind of operations should be decoupled from the application code, like event-based. What do you think?
There was a problem hiding this comment.
For this PoC I added it inline to show how policies could be created, but I agree that in a more complete design we should decouple it. Do you have any idea using an event-based approach?
There was a problem hiding this comment.
Instead of doing it in-line, we could hear the library creation event and react to it to create the policy, although not sure how sustainable this would be long term since we'd have to do this for all object lifecycle.
| """ | ||
| library = get_object_or_404(Library, id=pk) | ||
| library.delete() | ||
| enforcer.remove_filtered_policy(1, self.request.user.username, f"{self.request.path}{library.id}/", "") |
There was a problem hiding this comment.
In the architecture proposal I'm working on we're proposing to have an intermediate table which links policies to actual objects like libraries through a FK. When the object is removed then we should also remove the policy. With what you know do you think this setup makes sense?
| casbin-django-orm-adapter # Casbin Django ORM adapter | ||
| django_authorization # Django Authorization library | ||
| redis-watcher # Redis Watcher for Casbin |
There was a problem hiding this comment.
I'd like the dependency graph of this so we understand what each library is used for.
There was a problem hiding this comment.
With the changes I have made, the current requirements are:
casbin: Core authorization engine. Evaluates requests against model + policy.casbin-django-orm-adapter: Persists Casbin policies using Django ORM. Bridges Casbin with our database.redis-watcher: Keeps enforcers in sync across Django processes/workers by broadcasting policy changes through Redis.
The django-authorization library was removed. This library was used to integrates Casbin with Django by adding middleware and decorators for the views that automatically enforce Casbin policies during request handling.
There was a problem hiding this comment.
What I liked about django-authorization is that includes some useful commands: https://github.com/pycasbin/django-authorization/tree/master/dauthz/management/commands, I think we should also extend those to implement a "validate" for our rules. What do you think?
|
The implementation of the Casbin model continued in this PR: #49 |
github-project-automation
bot
moved this from Waiting on Author
to Done
in Contributions
3 participants
Description
DO NOT MERGE. Only some basic tests using Casbin.
Endpoints
/openedx-authz/api/libraries/: List all Libraries (according to a custom model). Initially, only the superadmin users have permissions./openedx-authz/api/admin-roles/: Add or remove users to grant access to all resources./openedx-authz/api/policy-single/: Add or remove specific permissions to users in a specific resource.How to test
Using Tutor:
casbin_ruletable.{lms-domain}/openedx-authz/api/libraries/: List, create, update, or delete a Library.{lms-domain}/openedx-authz/api/admin-roles/: The user has access to all endpoints.{lms-domain}/openedx-authz/api/policy-bulk/(Currently, it doesn't work){lms-domain}/openedx-authz/api/policy-single/: The user has access to a specific endpoint.Demo
casbin-testing.mp4