fix: deny "modify tag" permission when tag.taxonomy is None

Author: bradenmacdonald

openedx/core/djangoapps/content_tagging/rules.py

@@ -355,9 +355,10 @@ def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None)
             return False  # Cannot edit tags in read-only taxonomies.
         if taxonomy.allow_free_text:
             return False  # Cannot edit tags in free-text taxonomies.
-    # FIXME: properly block adding new tags in this case. (when tag=None, we don't have access to 'taxonomy', so the
-    # permissions check needs to pass a dummy tag object.)
-    # Also, for superusers, this permissions check is bypassed: https://github.com/openedx/openedx-core/issues/635
+    if taxonomy is None:
+        # Taxonomy shouldn't be None if we're editing any real tag.
+        # And for testing "add", we should always be passed a dummy Tag() object with a valid taxonomy.
+        return False
     return oel_tagging.is_taxonomy_admin(user)
 
 # Taxonomy

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

@@ -365,13 +365,15 @@ def test_view_taxonomy_disabled_no_org(self):
     )
     def test_tag_base_edit_permissions(self, perm):
         """
-        Test that only Staff & Superuser can call add/edit/delete tags.
+        Test that only Staff & Superuser can call add/edit/delete tags in an
+        "all orgs" taxonomy.
         """
-        assert self.superuser.has_perm(perm)
-        assert self.staff.has_perm(perm)
-        assert not self.user_both_orgs.has_perm(perm)
-        assert not self.user_org2.has_perm(perm)
-        assert not self.learner.has_perm(perm)
+        tag = Tag(taxonomy=self.taxonomy_all_orgs)
+        assert self.superuser.has_perm(perm, tag)
+        assert self.staff.has_perm(perm, tag)
+        assert not self.user_both_orgs.has_perm(perm, tag)
+        assert not self.user_org2.has_perm(perm, tag)
+        assert not self.learner.has_perm(perm, tag)
 
     def test_tag_base_view_permissions(self):
         """
@@ -459,14 +461,14 @@ def test_free_text_taxonomy_tag(self, perm):
         "oel_tagging.delete_tag",
     )
     def test_tag_no_taxonomy(self, perm):
-        """Taxonomy administrators can modify any Tag, even those with no Taxonnmy."""
+        """A "floating" tag with no taxonomy cannot be edited. This shouldn't really happen"""
         tag = Tag()
 
-        # Global Taxonomy Admins can do pretty much anything
+        # superusers cannot be prevented from doing anything - their permissions check short-circuits our logic.
         assert self.superuser.has_perm(perm, tag)
-        assert self.staff.has_perm(perm, tag)
 
         # Everyone else can't do anything
+        assert not self.staff.has_perm(perm, tag)
         assert not self.user_both_orgs.has_perm(perm, tag)
         assert not self.user_org2.has_perm(perm, tag)
         assert not self.learner.has_perm(perm, tag)