From d81f0b131d413b587d1e251bc25cc32e35376335 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 13 Feb 2024 05:23:49 +1030 Subject: [PATCH 1/2] 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. This change adds a rules check for this case, and updates the tests. --- .../content_tagging/rest_api/v1/filters.py | 4 +-- .../rest_api/v1/tests/test_views.py | 35 +++++++++++-------- .../core/djangoapps/content_tagging/rules.py | 30 +++++++++++++++- .../content_tagging/tests/test_rules.py | 2 +- 4 files changed, 53 insertions(+), 18 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/filters.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/filters.py index cbf947612fc..e4fa403fa52 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/filters.py +++ b/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') 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 7b3efd9b4fa..76974a54748 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 @@ -125,6 +125,11 @@ def _setUp_users(self): email="staff@example.com", is_staff=True, ) + self.superuser = User.objects.create( + username="superuser", + email="superuser@example.com", + 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), diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 67623bef22b..9c920a9da2d 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -274,6 +274,31 @@ 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: + is_all_org, taxonomy_orgs = TaxonomyOrg.get_organizations(perm_obj.taxonomy) + if not is_all_org: + # object_id is valid, else it would have thrown in can_change_object_tag_objectid + context_key = get_context_key_from_key_string(perm_obj.object_id) + assert context_key.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 +329,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. diff --git a/openedx/core/djangoapps/content_tagging/tests/test_rules.py b/openedx/core/djangoapps/content_tagging/tests/test_rules.py index d64fd3449ea..d53256f510a 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_rules.py +++ b/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) From 86d28fad17dfc245b633aecbce5d2580526eb58e Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 15 Feb 2024 11:20:11 +1030 Subject: [PATCH 2/2] fix: address PR review --- openedx/core/djangoapps/content_tagging/rules.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 101c1b9087d..47638f2ebef 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -285,12 +285,16 @@ def can_change_object_tag( """ if oel_tagging.can_change_object_tag(user, perm_obj): if perm_obj and perm_obj.taxonomy and perm_obj.object_id: - is_all_org, taxonomy_orgs = TaxonomyOrg.get_organizations(perm_obj.taxonomy) - if not is_all_org: - # object_id is valid, else it would have thrown in can_change_object_tag_objectid + # 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