[FC-0099] feat: add model to be used as backreference to maintain rules consistent#100
[FC-0099] feat: add model to be used as backreference to maintain rules consistent#100mariajgrimaldi merged 26 commits intomainfrom
Conversation
|
Thanks for the pull request, @mariajgrimaldi! 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. DetailsWhere 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. |
747652c to
42d62bb
Compare
42d62bb to
88f8695
Compare
bmtcril
left a comment
There was a problem hiding this comment.
A couple of open questions, I think:
- How to handle wildcard scopes
- How to handle objects that do have local database entries
I'm not sure that we have to support number 2, but wanted to raise if for cases like granting or denying access to Aspects dashboards based on RBAC roles or being able to query APIs for certain objects in a Student Information System that uses UUIDs as identifiers.
03646c9 to
162adf0
Compare
|
@bmtcril: thanks for raising this!
For wildcard scopes, like all libraries, I was thinking we could set the scope to None. That should be fine since deleting any library wouldn’t affect the user's Casbin rule, as it doesn’t reference a specific one. Does that make sense, or do you see another case we should consider?
I’m guessing you meant those that don't have local database entries, right? I think we’d still need some way to reference them, but their deletion flow might work differently, maybe event-based but managed in this app, so that when the upstream object is deleted, the local entry is also removed once we’re notified. |
|
@mariajgrimaldi that all makes sense to me! |
rodmgwgu
left a comment
There was a problem hiding this comment.
Thanks for the detailed documentation. I haven't finished reviewing the code, but here are my initial impressions:
At first, I didn't like the idea of introducing database models for tracking relations for rules, as in my mind, keeping the authz model decoupled and generic simplifies maintainability and extensibility.
But looking at consistency, I don't currently see a better/most optimal way of doing it other than this approach.
|
|
||
|
|
||
| @receiver(post_delete, sender=ExtendedCasbinRule) | ||
| def delete_casbin_rule_on_extended_rule_deletion(sender, instance, **kwargs): |
There was a problem hiding this comment.
Do you know what happens if, for example, we have multiple instances of the lms (for scalability reasons), will the post_delete cause this to be executed on all the instances?
As we are just calling .delete() on CasbinRule, this shouldn't be an issue, but would be good to know to take into account for future changes to this handler.
There was a problem hiding this comment.
@rodmgwgu Signals should take place in a single process, but there is a reload thread in all processes that currently refreshes the policy on a timer. In the future we're hoping to replace that with a more robust solution.
However there are some cases we should be aware of like bulk deletes where these signals are not fired for each object deleted.
There was a problem hiding this comment.
This is definitely another caveat. This approach only works for application-level operations that aren’t immediately translated into SQL, like bulk operations. However, I’m not too worried about it. We could add a command to clean the Casbin table and ensure the relationship tree stays consistent.
|
|
||
| The handler keeps authorization data symmetric with three common flows: | ||
| - Direct ExtendedCasbinRule deletes (API/UI) trigger removal of the linked CasbinRule. | ||
| - Cascades from `Scope` or `Subject` deletions clear their ExtendedCasbinRule rows and, via this handler, the matching CasbinRule entries. |
There was a problem hiding this comment.
Are you sure that when ExtendedCasbinRule gets deleted through a CASCADE action, the post_delete event will be triggered?
My understanding is that the cascade deletion is done at the database level, so I'm not sure Django has a way of detecting this? (I haven't tried this so I may be missing something)
There was a problem hiding this comment.
Yes! I was also wondering if this would get triggered and which cases we were actually covering, but I tried to be thorough in the integration tests to make sure we cover at least our main use cases.
162adf0 to
a11ae03
Compare
|
Thanks for your early review, @rodmgwgu! Another alternative we considered was making it event based, but we ran into a few blockers when thinking about it long term. We'd need lifecycle events for each scope to cascade deletes to the upstream Casbin rule. That's not terrible by itself, but failure handling would be much more complex than using a lower level approach like foreign keys, which Django manages automatically. It's definitely more coupled but also less error prone in my opinion. With the current approach, we're only deleting in one direction (CasbinRule -> ExtendedCasbinRule), which feels more manageable. The event based option isn't completely off the table though, since there are cases we can't handle through references, like the user retirement one (#110). Future scopes would just need to inherit from the base scope model, and Django would keep the Casbin table consistent, with the same caveats we already discussed, which would still apply if we went with the event based approach. |
1ad96dd to
92bfa4f
Compare
eaa743e to
9be04f5
Compare
| * Implement custom matcher to check for staff and superuser status. | ||
|
|
||
| 0.13.1 - 2025-11-10 | ||
| 0.13.1 - 2025-11-11 |
There was a problem hiding this comment.
I've been thinking the entire day that is the 10th.
Description
This PR introduces a consistency mechanism between Open edX domain objects (such as users and content libraries) and Casbin policies. The goal is to keep authorization data synchronized with the lifecycle of real objects, ensuring that no orphaned or stale policies remain after deletions.
Initially, the plan was to use an event-based approach that listened to Open edX lifecycle events. However, this approach was limited because it would require creating new events for the user lifecycle, the platform does not expose a custom User model, and these events would only be useful in the short term since the long-term plan was already to move to a back-reference model.
Because of these limitations, this implementation goes straight for the more sustainable option: a back-reference model that ensures transactional consistency at the Django application layer.
Core Components
Entity Relation Diagram
Details
Extended Casbin Rule Model
The new model,
ExtendedCasbinRule, extends Casbin's base policy and adds scope and subject references using Django foreign keys:When a resource is deleted, Django's cascade mechanism ensures all related entries are cleaned up automatically through a complete deletion chain:
Bidirectional Cascade Implementation
The implementation uses database-level CASCADE for parent→child relationships (ContentLibrary→Scope, User→Subject, Scope/Subject→ExtendedCasbinRule, CasbinRule→ExtendedCasbinRule via OneToOneField).
For the reverse direction (ExtendedCasbinRule→CasbinRule), we use a
post_deletesignal handler that fires for both direct.delete()calls and CASCADE deletions to delete the CasbinRule, preventing orphaned authorization rules. The handler includes recursion protection usingContextVarto prevent infinite loops.Registry-Based Polymorphic Design
The authorization system uses a registry pattern to avoid hardcoding foreign keys in the base models:
Scope,Subject) do not define resource-specific fieldsContentLibraryScope,UserSubject) register with a namespace identifierNew scope or subject types can be added in separate apps by simply subclassing and registering, without touching core models or requiring redeployment.
Signal Handler for Reverse Cascade
We use a
post_deletesignal handler to delete the CasbinRule when ExtendedCasbinRule is deleted. The signal handler:post_deletesignal fires after the deletion process, after the transaction commits (immediate commit is assumed since the deletion CasbinRule deletion happens outside an atomic block)Test Cases
This PR includes the
/integrationfolder covering all critical cascade deletion workflows, consider the following:CASE 1:
lib:OpenedX:CSPROBCASE 2:
Repeat the same steps, but in this case unassign the user from the role, then the extended model should be removed (along with the other records of subject / scope)
To run these test cases:
tutor dev exec lms bash pytest /openedx/openedx-authz/openedx_authz/tests/integration/Caveats
post_deletesignal, which fires for both direct.delete()calls and CASCADE deletions, but may not fire in all scenarios (e.g., raw SQL deletions, certain bulk operations using.update()or raw SQL)Alternatives Considered
Event-Based Consistency: As described in issue #74, we considered using Open edX events. However, content libraries have lifecycle events but users don't, requiring a mix of events and Django signals. This would create inconsistent behavior and lacks transactional guarantees.
Hybrid Strategy: Use the backreference model for data consistency (users, libraries) and events only for async cases like user retirement. This is flexible and our long-term direction, but deemed too complex for the MVP.
References