[FC-0099] feat: allow disabling auto-load policy based on settings#113
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. |
| auto_load_policy_interval = getattr(settings, "CASBIN_AUTO_LOAD_POLICY_INTERVAL", -1) | ||
| if auto_load_policy_interval != -1: | ||
| enforcer.start_auto_load_policy(auto_load_policy_interval) | ||
| enforcer.enable_auto_save(True) |
There was a problem hiding this comment.
Shouldn't this be also part of the internal checking?
There was a problem hiding this comment.
We probably don't want to default to -1 either, and may want to check that the interval is > 0 since I don't know what will happen at 0 but it might result in a tight loop of database requests.
There was a problem hiding this comment.
Thanks for your comments!
Finally, I implemented this code:
auto_load_policy_interval = getattr(settings, "CASBIN_AUTO_LOAD_POLICY_INTERVAL", 0)
if auto_load_policy_interval > 0:
enforcer.start_auto_load_policy(auto_load_policy_interval)
enforcer.enable_auto_save(True)
else:
# Disable auto-save to prevent unnecessary database writes
enforcer.enable_auto_save(False)If CASBIN_AUTO_LOAD_POLICY_INTERVAL is not greater than 0, the following actions are performed:
- The auto-loading of the policy is disabled.
- Auto-save is also disabled.
What do you think? @bmtcril @mariajgrimaldi
| self.assertIn(new_assignment, policies_after_auto_load) | ||
|
|
||
| @override_settings(CASBIN_AUTO_LOAD_POLICY_INTERVAL=-1) | ||
| def test_auto_load_disabled(self): |
There was a problem hiding this comment.
Can we also make sure there are no SQL queries run in the background against the DB during this?
There was a problem hiding this comment.
Yes, that's right! Thanks. I updated the unit test to check the queries.
|
I think we could bump to 0.8.0 with these latest changes. What do you think? |
bmtcril
left a comment
There was a problem hiding this comment.
Changes make sense to me, thanks!
rodmgwgu
left a comment
There was a problem hiding this comment.
Code looks good, could you expand on which cases we would want to disable auto-load?
|
@rodmgwgu right now RBAC only works with the v2 libraries, so if a site isn't using that feature the auto reloading is an unnecessary burden on the database. This will just let sites that aren't using the feature have a way to get away from the extra unnecessary queries. |
Description
This PR adds the possibility to disable the auto-load of policies by setting the
CASBIN_AUTO_LOAD_POLICY_INTERVAL = -1.When disabled, no policy is loaded, and auto-save is also disabled; therefore, permission validation will always return False.
Merge Checklist:
Check off if complete or not applicable: