-
Notifications
You must be signed in to change notification settings - Fork 4
feat: Migration for legacy library permissions #134
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, @rodmgwgu! 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. 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. |
| role = LIBRARY_USER | ||
| else: | ||
| # This should not happen, log and skip | ||
| # TODO: Should we fail here instead? |
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.
Should we fail here if acess_level doesn't match the expected values? it shouldn't happen though as the model specifies those options only.
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 suppose it's possible that some earlier version of the model had different values that never got upgraded, but it seems unlikely. Failing a migration could leave the whole system in a bad state with no easy way to recover, so I think the current behavior is good.
mariajgrimaldi
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.
Thank you so much for this! How do you think would be an efficient way of testing this in a remote environment? I think we can use a simple script to populate the ContentLibraryPermission and then run this migration to see how well it behaves. What do you think?
Good Idea, so far I've doing this manually, I'll create a script for testing this. |
I created a script to test this, the full instructions are on the gist here: https://gist.github.com/rodmgwgu/74df8f048247de0c0752ccddb2bb194d In includes 3 functions to be run on the django shell environment, one creates the test data prior to migration (setup), other verifies that the roles were correctly assigned after migration (verify), and other for clenaup. |
|
Sorry for the delay! I'll be testing this today :) |
| ) | ||
|
|
||
|
|
||
| def migrate_legacy_permissions(apps, schema_editor): |
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.
[non-blocking] When figuring out how to test this in a remote environment (I don't have easy access to the shell, so I was bootstrapping everything first), and I thought we could turn it into a testable utility that's imported here and also optionally used as a management command. I was creating one to test it remotely, and that’s where the idea came from. What do you think?
For testing with the content library model, you could use a stub mechanism like this one: https://github.com/openedx/openedx-authz/pull/100/files#diff-51f357adcfaba44382e81a2e9c58a403e4153752df0070b86e7dbc2b2f580727
|
I asked for help running the script, but my run was a bit clumsy so I could not get it to work. I am pretty sure the problem was my setup and I need to try again. Here is what I got when I ran it: app@lms-59cfc79b4c-nzhdz:~/edx-platform$ python manage.py lms mock_migrate verify
2025-11-11 21:32:58,549 WARNING 78 [py.warnings] [user None] [ip None] warnings.py:110 - /openedx/venv/lib/python3.11/site-packages/fs/__init__.py:4: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
__import__("pkg_resources").declare_namespace(__name__)
2025-11-11 21:32:58,673 WARNING 78 [py.warnings] [user None] [ip None] warnings.py:110 - /openedx/venv/lib/python3.11/site-packages/fs/__init__.py:4: DeprecationWarning: Deprecated call to `pkg_resources.declare_namespace('fs')`.
Implementing implicit namespace packages (as specified in PEP 420) is preferred to `pkg_resources.declare_namespace`. See https://setuptools.pypa.io/en/latest/references/keywords.html#keyword-namespace-packages
__import__("pkg_resources").declare_namespace(__name__)
2025-11-11 21:32:58,674 WARNING 78 [py.warnings] [user None] [ip None] warnings.py:110 - /openedx/venv/lib/python3.11/site-packages/fs/opener/__init__.py:6: DeprecationWarning: Deprecated call to `pkg_resources.declare_namespace('fs.opener')`.
Implementing implicit namespace packages (as specified in PEP 420) is preferred to `pkg_resources.declare_namespace`. See https://setuptools.pypa.io/en/latest/references/keywords.html#keyword-namespace-packages
__import__("pkg_resources").declare_namespace(__name__)
2025-11-11 21:32:58,675 WARNING 78 [py.warnings] [user None] [ip None] warnings.py:110 - /openedx/venv/lib/python3.11/site-packages/pkg_resources/__init__.py:2331: DeprecationWarning: Deprecated call to `pkg_resources.declare_namespace('fs')`.
Implementing implicit namespace packages (as specified in PEP 420) is preferred to `pkg_resources.declare_namespace`. See https://setuptools.pypa.io/en/latest/references/keywords.html#keyword-namespace-packages
declare_namespace(parent)
2025-11-11 21:33:00,551 INFO 78 [casbin_adapter.enforcer] [user None] [ip None] enforcer.py:21 - Deferring casbin enforcer initialisation until django is ready
2025-11-11 21:33:02,480 WARNING 78 [py.warnings] [user None] [ip None] warnings.py:110 - /openedx/venv/lib/python3.11/site-packages/drf_yasg/views.py:84: DeprecationWarning: SwaggerJSONRenderer & SwaggerYAMLRenderer's `format` has changed to not include a `.` prefix, please silence this warning by setting `SWAGGER_USE_COMPAT_RENDERERS = False` in your Django settings and ensure your application works (check your URLCONF and swagger/redoc URLs).
warnings.warn(
System check identified some issues:
WARNINGS:
embargo.GlobalRestrictedCountry.country: (fields.W342) Setting unique=True on a ForeignKey has the same effect as using a OneToOneField.
HINT: ForeignKey(unique=True) is usually better served by a OneToOneField.
2025-11-11 21:33:03,312 INFO 78 [casbin_adapter.enforcer] [user None] [ip None] enforcer.py:25 - Performing deferred casbin enforcer initialisation
2025-11-11 21:33:03,312 INFO 78 [casbin.policy] [user None] [ip None] model.py:112 - Model:
2025-11-11 21:33:03,312 INFO 78 [casbin.policy] [user None] [ip None] model.py:115 - r.r: sub, act, scope
2025-11-11 21:33:03,313 INFO 78 [casbin.policy] [user None] [ip None] model.py:115 - p.p: sub, act, scope, eft
2025-11-11 21:33:03,313 INFO 78 [casbin.policy] [user None] [ip None] model.py:115 - e.e: some(where (p_eft == allow)) && !some(where (p_eft == deny))
2025-11-11 21:33:03,313 INFO 78 [casbin.policy] [user None] [ip None] model.py:115 - m.m: is_staff_or_superuser(r_sub, r_act, r_scope) || (g(r_sub, p_sub, r_scope) || g(r_sub, p_sub, "*")) && keyMatch(r_scope, p_scope) && (r_act == p_act || g2(p_act, r_act))
2025-11-11 21:33:03,313 INFO 78 [casbin.policy] [user None] [ip None] model.py:115 - g.g: _, _, _
2025-11-11 21:33:03,313 INFO 78 [casbin.policy] [user None] [ip None] model.py:115 - g.g2: _, _
2025-11-11 21:33:03,315 INFO 78 [casbin.policy] [user None] [ip None] policy.py:73 - Policy:
2025-11-11 21:33:03,316 INFO 78 [casbin.policy] [user None] [ip None] policy.py:79 - p : sub, act, scope, eft : [['role^library_admin', 'act^view_library', 'lib^*', 'allow'], ...]
2025-11-11 21:33:03,316 INFO 78 [casbin.policy] [user None] [ip None] policy.py:79 - g : _, _, _ : [['user^library_admin_user', 'role^library_admin', 'lib^lib:OpenedX:CSPROB'], ...]
2025-11-11 21:33:03,316 INFO 78 [casbin.policy] [user None] [ip None] policy.py:79 - g2 : _, _ : [['act^manage_library_tags', 'act^edit_library_content'], ...]
Traceback (most recent call last):
File "/openedx/edx-platform/manage.py", line 99, in <module>
execute_from_command_line([sys.argv[0]] + django_args)
File "/openedx/venv/lib/python3.11/site-packages/django/core/management/__init__.py", line 442, in execute_from_command_line
utility.execute()
File "/openedx/venv/lib/python3.11/site-packages/django/core/management/__init__.py", line 436, in execute
self.fetch_command(subcommand).run_from_argv(self.argv)
File "/openedx/venv/lib/python3.11/site-packages/django/core/management/base.py", line 412, in run_from_argv
self.execute(*args, **cmd_options)
File "/openedx/venv/lib/python3.11/site-packages/django/core/management/base.py", line 458, in execute
output = self.handle(*args, **options)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/openedx/venv/lib/python3.11/site-packages/openedx_authz/management/commands/mock_migrate.py", line 216, in handle
verify_data()
File "/openedx/venv/lib/python3.11/site-packages/openedx_authz/management/commands/mock_migrate.py", line 130, in verify_data
assert len(assignments) == 1
^^^^^^^^^^^^^^^^^^^^^
AssertionErrorSince I do not have easy access to the Django shell in this sandbox, I added a management command to run the migration instead: My plan is to call it from a Tutor job to reproduce and debug the migration. |
|
I couldn't figure out why I wasn't finding the correct logs so I dropped the verification step and the final cleanup. The database is now in an inconsistent state but it's enough to confirm that the migration works! Here are the utilities I used to test this out: https://github.com/eduNEXT/tutor-contrib-authz/blob/main/tutorauthz/templates/authz/tasks/lms/init |
a8f86ab to
9a4b6a7
Compare
|
I added the migration test, but the quality command is failing with a "AstroidError" when trying to validate an import from django.contrib.auth.models. Have you seen this before @mariajgrimaldi @bmtcril Otherwise, the tests are passing in my local. |
mariajgrimaldi
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.
This looks great! Again, thank you so much for taking this on!
|
Can we update the changelog and the init file? Thanks! |
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.
I think we still want #142 to land and to update the permission names before we merge this? Does that sound right @mariajgrimaldi ?
d4863cc to
5e4feed
Compare
5e4feed to
e306b71
Compare
|
I think that would be enough, can you run tests locally to make sure everything works correctly? |
e306b71 to
b4c7282
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.
Looks good, thank you!
b4c7282 to
c8d40ab
Compare





Adds data migration to migrate legacy Library permissions to the new authz system.
The old Library permissions are stored in the ContentLibraryPermission model, it consists of the following columns:
In the new Authz model, this would roughly translate to:
Now, we don't have an equivalent concept to "Group", for this we will go through the users in the group and assign roles independently.
Related issue: #89
How to test
Follow the instructions on this gist: https://gist.github.com/rodmgwgu/74df8f048247de0c0752ccddb2bb194d
Merge checklist:
Check off if complete or not applicable: