fix: prevent cross-org ObjectTags from being created (#633)

* fix: prevent cross-org ObjectTags from being created

A "cross-org" ObjectTag is when the object_id references an org that is not in the taxonomy's allowed list of orgs.

Similarly, we forbid creating object tags for a taxonomy with no allowed orgs listed.

openedx/core/djangoapps/content_tagging/rest_api/v1/filters.py

@@ -65,7 +65,7 @@ class ObjectTagTaxonomyOrgFilterBackend(BaseFilterBackend):
 
     def filter_queryset(self, request, queryset, _):
         if oel_tagging.is_taxonomy_admin(request.user):
-            return queryset
+            return queryset.prefetch_related('taxonomy__taxonomyorg_set')
 
         user_admin_orgs = get_admin_orgs(request.user)
         user_orgs = get_user_orgs(request.user)
@@ -87,4 +87,4 @@ def filter_queryset(self, request, queryset, _):
                     )
                 )
             )
-        )
+        ).prefetch_related('taxonomy__taxonomyorg_set')

openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py

@@ -125,6 +125,11 @@ def _setUp_users(self):
             email="[email protected]",
             is_staff=True,
         )
+        self.superuser = User.objects.create(
+            username="superuser",
+            email="[email protected]",
+            is_superuser=True,
+        )
 
         self.staffA = User.objects.create(
             username="staffA",
@@ -1652,14 +1657,15 @@ def test_tag_library_invalid(self, user_attr, taxonomy_attr):
         assert response.status_code == status.HTTP_400_BAD_REQUEST
 
     @ddt.data(
-        ("staff", status.HTTP_200_OK),
+        ("superuser", status.HTTP_200_OK),
+        ("staff", status.HTTP_403_FORBIDDEN),
         ("staffA", status.HTTP_403_FORBIDDEN),
         ("staffB", status.HTTP_403_FORBIDDEN),
     )
     @ddt.unpack
     def test_tag_cross_org(self, user_attr, expected_status):
         """
-        Tests that only global admins can add a taxonomy from orgA to an object from orgB
+        Tests that only superusers may add a taxonomy from orgA to an object from orgB
         """
         user = getattr(self, user_attr)
         self.client.force_authenticate(user=user)
@@ -1671,14 +1677,15 @@ def test_tag_cross_org(self, user_attr, expected_status):
         assert response.status_code == expected_status
 
     @ddt.data(
-        ("staff", status.HTTP_200_OK),
+        ("superuser", status.HTTP_200_OK),
+        ("staff", status.HTTP_403_FORBIDDEN),
         ("staffA", status.HTTP_403_FORBIDDEN),
         ("staffB", status.HTTP_403_FORBIDDEN),
     )
     @ddt.unpack
     def test_tag_no_org(self, user_attr, expected_status):
         """
-        Tests that only global admins can add a no-org taxonomy to an object
+        Tests that only superusers may add a no-org taxonomy to an object
         """
         user = getattr(self, user_attr)
         self.client.force_authenticate(user=user)
@@ -1771,15 +1778,15 @@ def test_get_tags(self):
         assert response3.data[str(self.courseA)]["taxonomies"] == expected_tags
 
     @ddt.data(
-        ('staff', 'courseA', 7),
-        ('staff', 'libraryA', 7),
-        ("content_creatorA", 'courseA', 13, False),
-        ("content_creatorA", 'libraryA', 13, False),
-        ("library_staffA", 'libraryA', 13, False),  # Library users can only view objecttags, not change them?
-        ("library_userA", 'libraryA', 13, False),
-        ("instructorA", 'courseA', 13),
-        ("course_instructorA", 'courseA', 13),
-        ("course_staffA", 'courseA', 13),
+        ('staff', 'courseA', 8),
+        ('staff', 'libraryA', 8),
+        ("content_creatorA", 'courseA', 11, False),
+        ("content_creatorA", 'libraryA', 11, False),
+        ("library_staffA", 'libraryA', 11, False),  # Library users can only view objecttags, not change them?
+        ("library_userA", 'libraryA', 11, False),
+        ("instructorA", 'courseA', 11),
+        ("course_instructorA", 'courseA', 11),
+        ("course_staffA", 'courseA', 11),
     )
     @ddt.unpack
     def test_object_tags_query_count(
@@ -2322,7 +2329,7 @@ class TestTaxonomyTagsViewSet(TestTaxonomyObjectsMixin, APITestCase):
     """
     @ddt.data(
         ('staff', 11),
-        ("content_creatorA", 13),  # FIXME too many queries?
+        ("content_creatorA", 13),
         ("library_staffA", 13),
         ("library_userA", 13),
         ("instructorA", 13),

openedx/core/djangoapps/content_tagging/rules.py

@@ -274,6 +274,35 @@ def can_view_object_tag_objectid(user: UserType, object_id: str) -> bool:
     return bool(object_org) and (is_org_admin(user, object_org) or is_org_user(user, object_org))
 
 
+@rules.predicate
+def can_change_object_tag(
+    user: UserType, perm_obj: oel_tagging.ObjectTagPermissionItem | None = None
+) -> bool:
+    """
+    Returns True if the given user may change object tags with the given taxonomy + object_id.
+
+    Adds additional checks to ensure the taxonomy is available for use with the object_id's org.
+    """
+    if oel_tagging.can_change_object_tag(user, perm_obj):
+        if perm_obj and perm_obj.taxonomy and perm_obj.object_id:
+            # can_change_object_tag_objectid already checked that object_id is valid and has an org,
+            # so these statements will not fail. But we need to assert to keep the type checker happy.
+            try:
+                context_key = get_context_key_from_key_string(perm_obj.object_id)
+                assert context_key.org
+            except (ValueError, AssertionError):
+                return False  # pragma: no cover
+
+            is_all_org, taxonomy_orgs = TaxonomyOrg.get_organizations(perm_obj.taxonomy)
+            if not is_all_org:
+                # Ensure the object_id's org is among the allowed taxonomy orgs
+                object_org = rules_cache.get_orgs([context_key.org])
+                return bool(object_org) and object_org[0] in taxonomy_orgs
+
+        return True
+    return False
+
+
 @rules.predicate
 def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None) -> bool:
     """
@@ -304,7 +333,10 @@ def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None)
 rules.set_perm("oel_tagging.view_tag", rules.always_allow)
 
 # ObjectTag
-rules.set_perm("oel_tagging.can_tag_object", oel_tagging.can_change_object_tag)
+rules.set_perm("oel_tagging.add_objecttag", can_change_object_tag)
+rules.set_perm("oel_tagging.change_objecttag", can_change_object_tag)
+rules.set_perm("oel_tagging.delete_objecttag", can_change_object_tag)
+rules.set_perm("oel_tagging.can_tag_object", can_change_object_tag)
 
 # This perms are used in the tagging rest api from openedx_tagging that is exposed in the CMS. They are overridden here
 # to include Organization and objects permissions.

openedx/core/djangoapps/content_tagging/tests/test_rules.py

@@ -537,7 +537,7 @@ def test_object_tag_no_orgs(self, perm, tag_attr):
         """Only superusers can create/edit an ObjectTag with a no-org Taxonomy"""
         object_tag = getattr(self, tag_attr)
         assert self.superuser.has_perm(perm, object_tag)
-        assert self.staff.has_perm(perm, object_tag)
+        assert not self.staff.has_perm(perm, object_tag)
         assert not self.user_both_orgs.has_perm(perm, object_tag)
         assert not self.user_org2.has_perm(perm, object_tag)
         assert not self.learner.has_perm(perm, object_tag)