feat: migrate org scope access forward and rollback from/to authz policies#249
feat: migrate org scope access forward and rollback from/to authz policies#249mariajgrimaldi merged 19 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. |
cf6f212 to
749754c
Compare
39591db to
2375588
Compare
rodmgwgu
left a comment
There was a problem hiding this comment.
Code is looking good, apart from the existing comments. Also I see that there is a coverage issue.
|
@mariajgrimaldi just a minor comment, could you add something like the following to the PR description? Resolves: #222 This helps link the task to the PR. |
…lob or specific key)
f099f26 to
7ad67d0
Compare
dwong2708
left a comment
There was a problem hiding this comment.
LGTM! Just a non-blocking comment
openedx_authz/api/roles.py
Outdated
| """ | ||
| return [ | ||
| role_assignment for role_assignment in get_role_assignments() | ||
| if isinstance(role_assignment.scope, tuple(scope_types)) |
There was a problem hiding this comment.
nit: I think the tuple cast could be moved above the return to avoid casting on each iteration. Alternatively, this function could accept a tuple directly, and the caller would pass it as such.
There was a problem hiding this comment.
@mariajgrimaldi, thanks for this PR.
Thanks for the helpers added in the api, and kudos for the script for testing, it makes it easy to try this out quickly. ✨
Migration and rollback results:
=== Summary ===
✅ PASS All checks passed.
And it looks good in the Casbin table and the course role access table.
A minor thing I found is in the migration messages: if the registry already exists, the message shows an error (the command didn't create it because it already exists). I'm not sure we should show an error when the registry is there.
Examples:

Here I was playing, and I got the error because a rollback entry already exists.
Here, I ran the migration script with the delete flag, immediately after running it without it.
... I don't think it is a blocker, but it is something I found.
|
@MaferMazu: thanks for the review!
This design was already in place in the original PR introducing this feature, so the decision not to delete existing records was made there and might be out of scope for this PR. However, it's worth considering as an improvement for better debugging. Maybe we could split into success, errors, and warnings, but deletion would still happen only for the success list, since those were the records migrated. This way, the system state stays the same before and after the migration. |
Description
Resolves: #222
This PR adds org-level support to the forward and rollback migration scripts.
Forward migration
The previous implementation filtered out course access roles with an empty
course_id, which excluded org-level roles entirely. Changes:course_idif non-empty (course-level scope, most restrictive)orgif non-empty (org-glob external key)Rollback migration
The rollback got a bit more complex. The previous approach queried the Casbin table through scope model relationships, but org-level roles have no associated scope, so they'd be excluded.
I went a different route: fetch all valid role assignments (course or org level), filter by
course_idororg_id, then recreate the corresponding course access roles. I found it easier to use API calls here rather than ORM queries, though the tradeoff is that filtering moves from the DB to runtime. Open to suggestions since it's the biggest refactor in the PR.How to test
I put together some scripts to test this. Download them and follow the instructions in the README:
scripts-migrations-testing.zip
Merge checklist:
Check off if complete or not applicable: