-
Notifications
You must be signed in to change notification settings - Fork 4
[FC-0099] feat: implement custom matcher in casbin model #114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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. 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. |
3b0c476 to
dcffff2
Compare
70bffb1 to
75942e6
Compare
|
Bryann is off this week so I'll be pushing this forward! |
openedx_authz/engine/enforcer.py
Outdated
| SyncedEnforcer: Configured Casbin enforcer with adapter and auto-sync | ||
| """ | ||
| # Avoid circular import | ||
| from openedx_authz.engine.matcher import check_custom_conditions # pylint: disable=import-outside-toplevel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we can avoid this circular import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this and it didn't fail. I'll deploy this so I can test it remotely!
openedx_authz/engine/enforcer.py
Outdated
|
|
||
| adapter = ExtendedAdapter() | ||
| enforcer = SyncedEnforcer(settings.CASBIN_MODEL, adapter) | ||
| enforcer.add_function("custom_check", check_custom_conditions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could call this: is_staff_or_superuser
| ("lib:AnyOrg1:ANYLIB1", True), | ||
| ("lib:AnyOrg2:ANYLIB2", True), | ||
| ("lib:AnyOrg3:ANYLIB3", True), | ||
| ("sc:AnyScope1", False), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note that I think sc here is out of date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True! I updated the tesr cases
| User.objects.create_superuser(username="superuser", email="super@example.com", password="test") | ||
| User.objects.create_user(username="regular_user", email="regular@example.com", password="test") | ||
|
|
||
| @data( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a test that handles non-existent library keys? I assume that would return False, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I added a test case here: 7805698
3be701c to
7805698
Compare
bmtcril
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
rodmgwgu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks
MaferMazu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this.
804f5a2 to
b5de0c8
Compare
Description
This PR adds a custom matcher to the Casbin model. The matcher allows validating permissions based on library or user attributes, similar to how it’s currently handled with bridgekeeper.
For now, it only checks whether the user is staff or a superuser to grant all permissions.
This implementation is based on the work done in #101
Related Issues
Testing Instructions
Using Tutor:
tutor dev exec lms python manage.py lms migratetutor dev exec lms python manage.py lms load_policiestutor dev exec lms python manage.py lms enforcementand verify that staff and superuser users have granted permissions in any library in any action.Merge Checklist:
Check off if complete or not applicable: