diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index fce2ce5ec4f6..1be452869399 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -336,15 +336,24 @@ def get_metadata(queryset: QuerySet[ContentLibrary], text_search: str | None = N return libraries -def require_permission_for_library_key(library_key: LibraryLocatorV2, user: UserType, permission) -> ContentLibrary: +def require_permission_for_library_key( + library_key: LibraryLocatorV2, user: UserType, permission: str | authz_api.data.PermissionData +) -> ContentLibrary: """ - Given any of the content library permission strings defined in - openedx.core.djangoapps.content_libraries.permissions, - check if the given user has that permission for the library with the - specified library ID. + Check if the user has the specified permission for a content library. - Raises django.core.exceptions.PermissionDenied if the user doesn't have - permission. + Args: + library_key: The library key identifying the content library + user: The user whose permissions are being checked + permission: Either a permission string from content_libraries.permissions + or a PermissionData instance from the authz API + + Returns: + ContentLibrary: The library object if permission check passes + + Raises: + ContentLibraryNotFound: If the library with the given key doesn't exist + PermissionDenied: If the user doesn't have the required permission """ library_obj = ContentLibrary.objects.get_by_key(library_key) # obj should be able to read any valid model object but mypy thinks it can only be @@ -828,6 +837,8 @@ def _transform_authz_permission_to_legacy_lib_permission(permission: str) -> str authz_permissions.CREATE_LIBRARY_COLLECTION.identifier: permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, authz_permissions.EDIT_LIBRARY_COLLECTION.identifier: permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, authz_permissions.DELETE_LIBRARY_COLLECTION.identifier: permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, + authz_permissions.MANAGE_LIBRARY_TAGS.identifier: permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, + authz_permissions.REUSE_LIBRARY_CONTENT.identifier: permissions.CAN_VIEW_THIS_CONTENT_LIBRARY, }.get(permission, permission) diff --git a/openedx/core/djangoapps/content_libraries/rest_api/libraries.py b/openedx/core/djangoapps/content_libraries/rest_api/libraries.py index 3c531d629706..63a8f5fccd0d 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/libraries.py @@ -575,7 +575,7 @@ def post(self, request, lib_key_str): Import the contents of the user's clipboard and paste them into the Library """ library_key = LibraryLocatorV2.from_string(lib_key_str) - api.require_permission_for_library_key(library_key, request.user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) + api.require_permission_for_library_key(library_key, request.user, authz_permissions.EDIT_LIBRARY_CONTENT) try: result = api.import_staged_content_from_user_clipboard(library_key, request.user) diff --git a/openedx/core/djangoapps/content_staging/views.py b/openedx/core/djangoapps/content_staging/views.py index 37bfa54d7df2..79e5d8f05552 100644 --- a/openedx/core/djangoapps/content_staging/views.py +++ b/openedx/core/djangoapps/content_staging/views.py @@ -11,6 +11,7 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locator import CourseLocator, LibraryLocatorV2 +from openedx_authz.constants import permissions as authz_permissions from rest_framework.exceptions import NotFound, PermissionDenied, ValidationError from rest_framework.response import Response from rest_framework.views import APIView @@ -114,7 +115,7 @@ def post(self, request): lib_api.require_permission_for_library_key( course_key, request.user, - lib_api.permissions.CAN_VIEW_THIS_CONTENT_LIBRARY + authz_permissions.REUSE_LIBRARY_CONTENT ) block = xblock_api.load_block(usage_key, user=None) version_num = lib_api.get_library_block(usage_key).draft_version_num diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index b40ad5d55156..b5d7e733448e 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -7,7 +7,7 @@ import abc import json from io import BytesIO -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch from urllib.parse import parse_qs, urlparse import ddt @@ -36,6 +36,7 @@ from openedx.core.djangoapps.content_tagging import api as tagging_api from openedx.core.djangoapps.content_tagging.models import TaxonomyOrg from openedx.core.djangolib.testing.utils import skip_unless_cms +from openedx_authz.constants import permissions as authz_permissions from ....tests.test_objecttag_export_helpers import TaggedCourseMixin @@ -1624,6 +1625,35 @@ def test_tag_library(self, user_attr, taxonomy_attr, tag_values, expected_status assert status.is_success(new_response.status_code) assert new_response.data == response.data + @ddt.data("libraryA", "collection_key", "container_key") + @patch("openedx_authz.api.is_user_allowed") + @patch("openedx.core.djangoapps.content_tagging.rules.has_studio_write_access") + def test_tag_library_objects_with_manage_library_tags_permission( + self, + object_attr, + mock_has_studio_write_access, + mock_is_user_allowed, + ): + """ + Users with MANAGE_LIBRARY_TAGS permission should be able to tag: + - the library itself + - collections in the library + - containers in the library + """ + mock_is_user_allowed.return_value = True + object_id = getattr(self, object_attr) + + self.client.force_authenticate(user=self.library_userA) + response = self._call_put_request(object_id, self.tA1.pk, ["Tag 1"]) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + mock_is_user_allowed.assert_called_with( + self.library_userA.username, + authz_permissions.MANAGE_LIBRARY_TAGS.identifier, + self.libraryA, + ) + mock_has_studio_write_access.assert_not_called() + @ddt.data( "staffA", "staff", @@ -1945,15 +1975,15 @@ def test_get_copied_tags(self): @ddt.data( ('staff', 'courseA', 8), - ('staff', 'libraryA', 8), - ('staff', 'collection_key', 8), + ('staff', 'libraryA', 17), + ('staff', 'collection_key', 17), ("content_creatorA", 'courseA', 18, False), - ("content_creatorA", 'libraryA', 18, False), - ("content_creatorA", 'collection_key', 18, False), - ("library_staffA", 'libraryA', 18, False), # Library users can only view objecttags, not change them? - ("library_staffA", 'collection_key', 18, False), - ("library_userA", 'libraryA', 18, False), - ("library_userA", 'collection_key', 18, False), + ("content_creatorA", 'libraryA', 23, False), + ("content_creatorA", 'collection_key', 23, False), + ("library_staffA", 'libraryA', 23, False), # Library users can only view objecttags, not change them? + ("library_staffA", 'collection_key', 23, False), + ("library_userA", 'libraryA', 23, False), + ("library_userA", 'collection_key', 23, False), ("instructorA", 'courseA', 18), ("course_instructorA", 'courseA', 18), ("course_staffA", 'courseA', 18), diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 3f798d4053ba..e20cb401c688 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -7,6 +7,9 @@ import django.contrib.auth.models import openedx_tagging.rules as oel_tagging import rules +from opaque_keys.edx.locator import LibraryLocatorV2 +from openedx_authz import api as authz_api +from openedx_authz.constants import permissions as authz_permissions from organizations.models import Organization from common.djangoapps.student.auth import has_studio_read_access, has_studio_write_access @@ -17,13 +20,12 @@ OrgContentCreatorRole, OrgInstructorRole, OrgLibraryUserRole, - OrgStaffRole + OrgStaffRole, ) from .models import TaxonomyOrg from .utils import check_taxonomy_context_key_org, get_context_key_from_key_string, rules_cache - UserType = Union[django.contrib.auth.models.User, django.contrib.auth.models.AnonymousUser] @@ -218,7 +220,13 @@ def can_change_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: @rules.predicate def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: """ - Everyone that has permission to edit the object should be able to tag it. + Return True if the user may add or modify tags on the given object. + + For Content Libraries V2, this requires either explicit library tagging permission + (MANAGE_LIBRARY_TAGS) or org-level admin access for the library's org. + + For other contexts (courses, xblocks, etc.), this requires studio write access or + org-level admin access for the object's org. """ if not object_id: return True @@ -229,6 +237,16 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: except (ValueError, AssertionError): return False + # For Content Libraries V2, prefer explicit library tagging permission, + # however, org-level admins are also allowed to perform these operations. + if isinstance(context_key, LibraryLocatorV2) and authz_api.is_user_allowed( + user.username, + authz_permissions.MANAGE_LIBRARY_TAGS.identifier, + str(context_key), + ): + return True + + # For other contexts (courses, xblocks, etc.), use general write or org-admin access if has_studio_write_access(user, context_key): return True @@ -276,7 +294,13 @@ def can_view_object_tag_objectid(user: UserType, object_id: str) -> bool: @rules.predicate def can_remove_object_tag_objectid(user: UserType, object_id: str) -> bool: """ - Everyone that has permission to edit the object should be able remove tags from it. + Return True if the user may remove tags from the given object. + + For Content Libraries V2, this requires either explicit library tagging permission + (MANAGE_LIBRARY_TAGS) or org-level admin access for the library's org. + + For other contexts (courses, xblocks, etc.), this requires studio write access or + org-level admin access for the object's org. """ if not object_id: raise ValueError("object_id must be provided") @@ -290,6 +314,16 @@ def can_remove_object_tag_objectid(user: UserType, object_id: str) -> bool: except (ValueError, AssertionError): return False + # For Content Libraries V2, prefer explicit library tagging permission, + # however, org-level admins are also allowed to perform these operations. + if isinstance(context_key, LibraryLocatorV2) and authz_api.is_user_allowed( + user.username, + authz_permissions.MANAGE_LIBRARY_TAGS.identifier, + str(context_key), + ): + return True + + # For other contexts (courses, xblocks, etc.), use general write or org-admin access if has_studio_write_access(user, context_key): return True diff --git a/openedx/core/djangoapps/content_tagging/tests/test_rules.py b/openedx/core/djangoapps/content_tagging/tests/test_rules.py index 75926d094103..b94dafa97d80 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_rules.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_rules.py @@ -1,9 +1,12 @@ """Tests content_tagging rules-based permissions""" +from unittest.mock import patch + import ddt from django.contrib.auth import get_user_model from django.test import TestCase -from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator +from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator, LibraryLocatorV2 +from openedx_authz.constants import permissions as authz_permissions from openedx_tagging.models import ( Tag, UserSystemDefinedTaxonomy, @@ -14,6 +17,7 @@ from common.djangoapps.student.roles import CourseStaffRole, OrgStaffRole from .. import api +from ..rules import can_change_object_tag_objectid, can_remove_object_tag_objectid from .test_api import TestTaxonomyMixin User = get_user_model() @@ -608,3 +612,137 @@ def test_view_object_tag_diabled(self): assert not self.user_both_orgs.has_perm(perm, self.disabled_course_tag) assert not self.user_org2.has_perm(perm, self.disabled_course_tag) assert not self.learner.has_perm(perm, self.disabled_course_tag) + + +class TestRulesLibraryV2Permissions(TestTaxonomyMixin, TestCase): + """ + Tests for Content Library V2 permissions with MANAGE_LIBRARY_TAGS. + """ + + def setUp(self): + super().setUp() + + self.library_user = User.objects.create( + username="library_user", + email="library_user@example.com", + ) + self.org_admin = User.objects.create( + username="org_admin", + email="org_admin@example.com", + ) + self.regular_user = User.objects.create( + username="regular_user", + email="regular_user@example.com", + ) + self.superuser = User.objects.create( + username="superuser", + email="superuser@example.com", + is_superuser=True, + ) + + # Make org_admin an OrgStaffRole for org1 + update_org_role( + self.superuser, + OrgStaffRole, + self.org_admin, + [self.org1.short_name], + ) + + self.library_key = LibraryLocatorV2.from_string(f"lib:{self.org1.short_name}:test_library") + + @patch("openedx_authz.api.is_user_allowed") + def test_change_objecttag_objectid_with_manage_library_tags_permission(self, mock_is_user_allowed): + """ + Test that a user with MANAGE_LIBRARY_TAGS permission can change tags on a library. + """ + mock_is_user_allowed.return_value = True + + result = can_change_object_tag_objectid(self.library_user, str(self.library_key)) + + self.assertTrue(result) + mock_is_user_allowed.assert_called_once_with( + self.library_user.username, + authz_permissions.MANAGE_LIBRARY_TAGS.identifier, + str(self.library_key), + ) + + @patch("openedx_authz.api.is_user_allowed") + def test_change_objecttag_objectid_without_manage_library_tags_but_org_admin(self, mock_is_user_allowed): + """ + Test that an org admin can change tags on a library even without + explicit MANAGE_LIBRARY_TAGS permission. + """ + mock_is_user_allowed.return_value = False + + result = can_change_object_tag_objectid(self.org_admin, str(self.library_key)) + + self.assertTrue(result) + + @patch("openedx_authz.api.is_user_allowed") + def test_change_objecttag_objectid_without_permissions(self, mock_is_user_allowed): + """ + Test that a regular user without MANAGE_LIBRARY_TAGS permission and + without org admin access cannot change tags. + """ + mock_is_user_allowed.return_value = False + + result = can_change_object_tag_objectid(self.regular_user, str(self.library_key)) + + self.assertFalse(result) + + @patch("openedx_authz.api.is_user_allowed") + def test_remove_objecttag_objectid_with_manage_library_tags_permission(self, mock_is_user_allowed): + """ + Test that a user with MANAGE_LIBRARY_TAGS permission can remove tags + from a library. + """ + mock_is_user_allowed.return_value = True + + result = can_remove_object_tag_objectid(self.library_user, str(self.library_key)) + + self.assertTrue(result) + mock_is_user_allowed.assert_called_once_with( + self.library_user.username, + authz_permissions.MANAGE_LIBRARY_TAGS.identifier, + str(self.library_key), + ) + + @patch("openedx_authz.api.is_user_allowed") + def test_remove_objecttag_objectid_without_manage_library_tags_but_org_admin(self, mock_is_user_allowed): + """ + Test that an org admin can remove tags from a library even without + explicit MANAGE_LIBRARY_TAGS permission. + """ + mock_is_user_allowed.return_value = False + + result = can_remove_object_tag_objectid(self.org_admin, str(self.library_key)) + + self.assertTrue(result) + + @patch("openedx_authz.api.is_user_allowed") + def test_remove_objecttag_objectid_without_permissions(self, mock_is_user_allowed): + """ + Test that a regular user without MANAGE_LIBRARY_TAGS permission and + without org admin access cannot remove tags. + """ + mock_is_user_allowed.return_value = False + + result = can_remove_object_tag_objectid(self.regular_user, str(self.library_key)) + + self.assertFalse(result) + + def test_invalid_library_key(self): + """ + Test that invalid library keys return False. + """ + self.assertFalse(can_change_object_tag_objectid(self.library_user, "invalid_key")) + self.assertFalse(can_remove_object_tag_objectid(self.library_user, "invalid_key")) + + def test_empty_object_id(self): + """ + Test behavior with empty object_id. + """ + self.assertTrue(can_change_object_tag_objectid(self.library_user, "")) + + with self.assertRaises(ValueError): + can_remove_object_tag_objectid(self.library_user, "")