diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py index 556ea06cfe1e..ef01b6930e52 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -9,7 +9,7 @@ from django.db.models import Count from django.http import StreamingHttpResponse from openedx_authz import api as authz_api -from openedx_authz.constants.permissions import COURSES_MANAGE_TAGS, COURSES_VIEW_COURSE +from openedx_authz.constants.permissions import COURSES_VIEW_COURSE from openedx_events.content_authoring.data import ContentObjectChangedData, ContentObjectData from openedx_events.content_authoring.signals import CONTENT_OBJECT_ASSOCIATIONS_CHANGED, CONTENT_OBJECT_TAGS_CHANGED from openedx_tagging import rules as oel_tagging_rules @@ -207,24 +207,6 @@ def ensure_has_view_object_tag_permission(self, user, taxonomy, object_id): # Fall back to parent implementation super().ensure_has_view_object_tag_permission(user, taxonomy, object_id) - def ensure_user_has_can_tag_object_permissions(self, user, tags_data, object_id): - """ - Check if user has permission to tag object for each taxonomy in tags_data. - - This method is overridden to conditionally use openedx-authz when the toggle is enabled. - - When using openedx-authz, if the user has manage tags permission for the course, - they can tag the object regardless of the taxonomy. - """ - should_use_authz, course_key = self._authz_check - if should_use_authz and not authz_api.is_user_allowed( - user.username, COURSES_MANAGE_TAGS.identifier, str(course_key) - ): - raise PermissionDenied("You do not have permission to manage object tags.") - if not should_use_authz: - # Fall back to parent implementation - super().ensure_user_has_can_tag_object_permissions(user, tags_data, object_id) - def update(self, request, *args, **kwargs) -> Response: """ Extend the update method to fire CONTENT_OBJECT_ASSOCIATIONS_CHANGED event diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 3b70076189ef..33dd13c18fe9 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -23,6 +23,7 @@ OrgStaffRole, ) +from .auth import should_use_course_authz_for_object from .models import TaxonomyOrg from .utils import check_taxonomy_context_key_org, get_context_key_from_key_string, rules_cache @@ -225,8 +226,10 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: 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. + For a course (or object within one) switched to openedx-authz, this requires + courses.manage_tags through that service alone, with no fallback to studio write or + org-admin access; for any other course, xblock, etc. not yet switched, those checks + still apply exactly as before. """ if not object_id: return True @@ -246,6 +249,12 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: ): return True + should_use_authz, course_key = should_use_course_authz_for_object(object_id) + if should_use_authz: + return authz_api.is_user_allowed( + user.username, authz_permissions.COURSES_MANAGE_TAGS.identifier, str(course_key) + ) + # 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 4432310147c7..e8f81f604f1e 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_rules.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_rules.py @@ -5,6 +5,7 @@ import ddt from django.contrib.auth import get_user_model from django.test import TestCase +from edx_toggles.toggles.testutils import override_waffle_flag 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 @@ -12,6 +13,7 @@ from common.djangoapps.student.auth import add_users, update_org_role from common.djangoapps.student.roles import CourseStaffRole, OrgStaffRole +from openedx.core.toggles import AUTHZ_COURSE_AUTHORING_FLAG from .. import api from ..rules import can_change_object_tag_objectid, can_remove_object_tag_objectid @@ -743,3 +745,127 @@ def test_empty_object_id(self): with self.assertRaises(ValueError): # noqa: PT027 can_remove_object_tag_objectid(self.library_user, "") + + +class TestRulesCourseAuthzPermissions(TestTaxonomyMixin, TestCase): + """ + Tests for can_change_object_tag_objectid's course-authz-aware branch. + """ + + def setUp(self): + super().setUp() + + self.superuser = User.objects.create( + username="superuser", + email="superuser@example.com", + is_superuser=True, + ) + self.authz_user = User.objects.create( + username="authz_user", + email="authz_user@example.com", + ) + self.legacy_user = User.objects.create( + username="legacy_user", + email="legacy_user@example.com", + ) + self.org_admin_user = User.objects.create( + username="org_admin_user", + email="org_admin_user@example.com", + ) + + self.course_key = CourseLocator.from_string("course-v1:OeX+DemoX+Demo_Course") + self.xblock_key = BlockUsageLocator( + course_key=self.course_key, + block_type='problem', + block_id='block_id', + ) + # Granted while the waffle flag is off, so this goes through the legacy role path. + add_users(self.superuser, CourseStaffRole(self.course_key), self.legacy_user) + # org1's short_name ("OeX") matches self.course_key's org, so this is an org-level + # admin for the course's org, not merely a course-level role. + update_org_role(self.superuser, OrgStaffRole, self.org_admin_user, [self.org1.short_name]) + + @override_waffle_flag(AUTHZ_COURSE_AUTHORING_FLAG, active=True) + @patch("openedx_authz.api.is_user_allowed") + def test_course_switched_authz_only_role_allowed(self, mock_is_user_allowed): + """ + A course switched to openedx-authz grants access via authz_api.is_user_allowed alone. + """ + mock_is_user_allowed.return_value = True + + result = can_change_object_tag_objectid(self.authz_user, str(self.course_key)) + + self.assertTrue(result) # noqa: PT009 + mock_is_user_allowed.assert_called_once_with( + self.authz_user.username, + authz_permissions.COURSES_MANAGE_TAGS.identifier, + str(self.course_key), + ) + + @override_waffle_flag(AUTHZ_COURSE_AUTHORING_FLAG, active=True) + @patch("openedx_authz.api.is_user_allowed") + def test_course_switched_legacy_only_role_denied(self, mock_is_user_allowed): + """ + A switched course does not fall back to a legacy role: the switch is exclusive, not + an OR with the legacy check. + """ + mock_is_user_allowed.return_value = False + + result = can_change_object_tag_objectid(self.legacy_user, str(self.course_key)) + + self.assertFalse(result) # noqa: PT009 + mock_is_user_allowed.assert_called_once_with( + self.legacy_user.username, + authz_permissions.COURSES_MANAGE_TAGS.identifier, + str(self.course_key), + ) + + @override_waffle_flag(AUTHZ_COURSE_AUTHORING_FLAG, active=True) + @patch("openedx_authz.api.is_user_allowed") + def test_xblock_in_switched_course_authz_only_role_allowed(self, mock_is_user_allowed): + """ + An xblock inside a switched course resolves through the same authz-only branch as + the course itself: should_use_course_authz_for_object resolves the xblock's usage + key down to its course key. + """ + mock_is_user_allowed.return_value = True + + result = can_change_object_tag_objectid(self.authz_user, str(self.xblock_key)) + + self.assertTrue(result) # noqa: PT009 + mock_is_user_allowed.assert_called_once_with( + self.authz_user.username, + authz_permissions.COURSES_MANAGE_TAGS.identifier, + str(self.course_key), + ) + + @patch("openedx_authz.api.is_user_allowed") + def test_course_not_switched_legacy_role_allowed(self, mock_is_user_allowed): + """ + A course that hasn't been switched to openedx-authz keeps resolving through the + legacy role check, unaffected by this change. + """ + result = can_change_object_tag_objectid(self.legacy_user, str(self.course_key)) + + self.assertTrue(result) # noqa: PT009 + mock_is_user_allowed.assert_not_called() + + @override_waffle_flag(AUTHZ_COURSE_AUTHORING_FLAG, active=True) + @patch("openedx_authz.api.is_user_allowed") + def test_library_object_unchanged(self, mock_is_user_allowed): + """ + Library objects are unaffected by this branch: should_use_course_authz_for_object + only recognizes CourseKeys, so the library check still resolves through + MANAGE_LIBRARY_TAGS even with the course-authz flag on. + """ + mock_is_user_allowed.return_value = True + library_key = LibraryLocatorV2.from_string(f"lib:{self.org1.short_name}:test_library") + + result = can_change_object_tag_objectid(self.legacy_user, str(library_key)) + + self.assertTrue(result) # noqa: PT009 + mock_is_user_allowed.assert_called_once_with( + self.legacy_user.username, + authz_permissions.MANAGE_LIBRARY_TAGS.identifier, + str(library_key), + )