feat: add new content library enforcement points#38071
feat: add new content library enforcement points#38071BryanttV wants to merge 14 commits intoopenedx:masterfrom
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. |
3c811b1 to
20ab0d2
Compare
d976851 to
a12343b
Compare
a12343b to
d9f2892
Compare
| ("library_staffA", 'libraryA', 32, False), # Library users can only view objecttags, not change them? | ||
| ("library_staffA", 'collection_key', 32, False), | ||
| ("library_userA", 'libraryA', 32, False), | ||
| ("library_userA", 'collection_key', 32, False), |
There was a problem hiding this comment.
Do these tests have support for authz? It's not very crear to me how we're almost duplicating queries
This comment also relates to: https://github.com/openedx/openedx-platform/pull/38071/changes#r2906848050
There was a problem hiding this comment.
If not I think we should do a quick dive into why is this count is increasing so much (related: https://github.com/openedx/openedx-platform/pull/38071/changes#r2906848050) and add some tests to verify the new code path is actually working.
There was a problem hiding this comment.
I'm checking the additional queries.
There was a problem hiding this comment.
I did some testing and the queries increased as follows:
In the Staff and LibraryA test, it went from 8 to 17. These were the additional queries:
There are 4 queries to the cache policy table, 1 to the Casbin table, and 2 to user-related tables.
[7] (0.000s) SELECT "openedx_authz_policycachecontrol"."id", "openedx_authz_policycachecontrol"."version" FROM "openedx_authz_policycachecontrol" WHERE "openedx_authz_policycachecontrol"."id" = 1 LIMIT 21
[8] (0.000s) SAVEPOINT "s130125984304000_x2290"
[9] (0.000s) INSERT INTO "openedx_authz_policycachecontrol" ("id", "version") VALUES (1, '53167ddc5ae44a9eabc87c6a0667bec6') RETURNING "openedx_authz_policycachecontrol"."id"
[10] (0.000s) RELEASE SAVEPOINT "s130125984304000_x2290"
[11] (0.000s) SELECT "casbin_rule"."id", "casbin_rule"."ptype", "casbin_rule"."v0", "casbin_rule"."v1", "casbin_rule"."v2", "casbin_rule"."v3", "casbin_rule"."v4", "casbin_rule"."v5" FROM "casbin_rule"
[12] (0.000s) SELECT "auth_user"."id", "auth_user"."password", "auth_user"."last_login", "auth_user"."is_superuser", "auth_user"."username", "auth_user"."first_name", "auth_user"."last_name", "auth_user"."email", "auth_user"."is_staff", "auth_user"."is_active", "auth_user"."date_joined" FROM "auth_user" WHERE ("auth_user"."email" = 'staff' OR "auth_user"."username" = 'staff') LIMIT 21
[13] (0.000s) SELECT "user_api_userretirementrequest"."id", "user_api_userretirementrequest"."created", "user_api_userretirementrequest"."modified", "user_api_userretirementrequest"."user_id" FROM "user_api_userretirementrequest" WHERE "user_api_userretirementrequest"."user_id" = 2 LIMIT 21
[14] (0.000s) SELECT "openedx_authz_policycachecontrol"."id", "openedx_authz_policycachecontrol"."version" FROM "openedx_authz_policycachecontrol" WHERE "openedx_authz_policycachecontrol"."id" = 1 LIMIT 21
[15] (0.000s) SELECT "openedx_authz_policycachecontrol"."id", "openedx_authz_policycachecontrol"."version" FROM "openedx_authz_policycachecontrol" WHERE "openedx_authz_policycachecontrol"."id" = 1 LIMIT 21
For the ContentCreator and LibraryA test, the queries increased from 18 to 23. The additional queries are the following:
[17] (0.000s) SELECT "openedx_authz_policycachecontrol"."id", "openedx_authz_policycachecontrol"."version" FROM "openedx_authz_policycachecontrol" WHERE "openedx_authz_policycachecontrol"."id" = 1 LIMIT 21
[18] (0.000s) SELECT "auth_user"."id", "auth_user"."password", "auth_user"."last_login", "auth_user"."is_superuser", "auth_user"."username", "auth_user"."first_name", "auth_user"."last_name", "auth_user"."email", "auth_user"."is_staff", "auth_user"."is_active", "auth_user"."date_joined" FROM "auth_user" WHERE ("auth_user"."email" = 'content_creatorA' OR "auth_user"."username" = 'content_creatorA') LIMIT 21
[19] (0.000s) SELECT "user_api_userretirementrequest"."id", "user_api_userretirementrequest"."created", "user_api_userretirementrequest"."modified", "user_api_userretirementrequest"."user_id" FROM "user_api_userretirementrequest" WHERE "user_api_userretirementrequest"."user_id" = 6 LIMIT 21
[20] (0.000s) SELECT "openedx_authz_policycachecontrol"."id", "openedx_authz_policycachecontrol"."version" FROM "openedx_authz_policycachecontrol" WHERE "openedx_authz_policycachecontrol"."id" = 1 LIMIT 21
[21] (0.000s) SELECT "openedx_authz_policycachecontrol"."id", "openedx_authz_policycachecontrol"."version" FROM "openedx_authz_policycachecontrol" WHERE "openedx_authz_policycachecontrol"."id" = 1 LIMIT 21
There are 3 queries related to the policy cache and 2 queries to user-related tables. The latter occur because the authorization system retrieves the User object from the username. The same pattern occurs with the other tests.
There was a problem hiding this comment.
I added some additional unit tests
There was a problem hiding this comment.
Good! Thank you :))) these queries make sense to me!
| return False | ||
|
|
||
| # For Content Libraries V2, prefer explicit library tagging permission, | ||
| # but fall back to org-level admin access for backwards compatibility. |
There was a problem hiding this comment.
Is it backward compatibility or org-level course admins can also have access to these operations?
There was a problem hiding this comment.
Actually, you're right. Org-level admins also have access to these operations. I updated the comment, what do you think? 0679ec7
There was a problem hiding this comment.
Thanks so much for clarifying!
445388c to
f378617
Compare
|
Do you think it'd make sense to involve someone from product to approve these changes? If this makes sense to you, can we add a summary of the impact of this PR to share with them? (not sure if we did this already) Let me know! |
Description
This PR adds missing authorization enforcement points for Content Libraries v2, aligning tagging and reuse operations with the new granular authz permissions (notably
MANAGE_LIBRARY_TAGSandREUSE_LIBRARY_CONTENT). Currently, some operations on Content Libraries v2 rely on generic edit/view permissions instead of the more specific g2 authz permissionsWhat This PR Changes
New helper for library tagging access
has_library_tagging_access(user, library_key). This helper looks up theContentLibraryobject and checksMANAGE_LIBRARY_TAGSviauser_has_permission_across_lib_authz_systems.Updated tagging rules for library content
can_change_object_tag_objectidandcan_remove_object_tag_objectid. If the context is aLibraryLocatorV2, it now useshas_library_tagging_accessand therefore requiresMANAGE_LIBRARY_TAGS. For other contexts (courses, XBlocks, etc.), it still falls back tohas_studio_write_accessas before.New enforcement for content reuse flows
LibraryPasteClipboardViewnow requiresauthz_permissions.EDIT_LIBRARY_CONTENTinstead of the legacyCAN_EDIT_THIS_CONTENT_LIBRARYpermission.ClipboardEndpointnow requiresREUSE_LIBRARY_CONTENT(rather thanCAN_VIEW_THIS_CONTENT_LIBRARY) when bringing content from a library into a course.Authz-to-legacy permission mapping updates
_transform_authz_permission_to_legacy_lib_permissionis extended so that:MANAGE_LIBRARY_TAGS→CAN_EDIT_THIS_CONTENT_LIBRARYREUSE_LIBRARY_CONTENT→CAN_VIEW_THIS_CONTENT_LIBRARYSupporting information
Testing instructions
You can test the following endpoints:
REUSE_LIBRARY_CONTENTAll library roles (Library Admin, Library Author, Library Contributor, and Library User) can reuse library content:
{studio_domain}/api/content-staging/v1/clipboard/: Copy content to the clipboard.MANAGE_LIBRARY_TAGSAll library roles (except Library User) can manage (add or remove) tags for content in a library:
{studio_domain}/api/content_tagging/v1/object_tags/{object_key}/: Add or remove tags from library content.You can use the following Postman Collection.
Deadline
None