From b6366b67b37c7b53428efeda675a5e16cb498c38 Mon Sep 17 00:00:00 2001 From: Jillian Date: Sat, 17 Feb 2024 06:45:26 +1030 Subject: [PATCH] perf: reduce number of queries for content tagging endpoints (#34200) --- .../core/djangoapps/content_tagging/api.py | 3 +- .../djangoapps/content_tagging/models/base.py | 27 +-- .../content_tagging/rest_api/v1/filters.py | 15 +- .../rest_api/v1/serializers.py | 49 +++-- .../rest_api/v1/tests/test_views.py | 104 ++++++++--- .../content_tagging/rest_api/v1/views.py | 14 +- .../core/djangoapps/content_tagging/rules.py | 172 ++++++++++-------- .../content_tagging/tests/test_api.py | 4 +- .../content_tagging/tests/test_rules.py | 7 +- .../core/djangoapps/content_tagging/utils.py | 58 ++++++ requirements/constraints.txt | 2 +- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 15 files changed, 290 insertions(+), 173 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 629b380f97d2..4f62469220a5 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -87,7 +87,7 @@ def set_taxonomy_orgs( def get_taxonomies_for_org( enabled=True, - org_owner: Organization | None = None, + org_short_name: str | None = None, ) -> QuerySet: """ Generates a list of the enabled Taxonomies available for the given org, sorted by name. @@ -100,7 +100,6 @@ def get_taxonomies_for_org( If you want the disabled Taxonomies, pass enabled=False. If you want all Taxonomies (both enabled and disabled), pass enabled=None. """ - org_short_name = org_owner.short_name if org_owner else None return oel_tagging.get_taxonomies(enabled=enabled).filter( Exists( TaxonomyOrg.get_relationships( diff --git a/openedx/core/djangoapps/content_tagging/models/base.py b/openedx/core/djangoapps/content_tagging/models/base.py index 3ce125d99af3..8a232d3a7bf4 100644 --- a/openedx/core/djangoapps/content_tagging/models/base.py +++ b/openedx/core/djangoapps/content_tagging/models/base.py @@ -64,16 +64,21 @@ def get_relationships( @classmethod def get_organizations( - cls, taxonomy: Taxonomy, rel_type: RelType - ) -> list[Organization]: + cls, taxonomy: Taxonomy, rel_type=RelType.OWNER, + ) -> tuple[bool, list[Organization]]: """ - Returns the list of Organizations which have the given relationship to the taxonomy. + Returns a tuple containing: + * bool: flag indicating whether "all organizations" have the given relationship to the taxonomy + * orgs: list of Organizations which have the given relationship to the taxonomy """ - rels = cls.objects.filter( - taxonomy=taxonomy, - rel_type=rel_type, - ) - # A relationship with org=None means all Organizations - if rels.filter(org=None).exists(): - return list(Organization.objects.all()) - return [rel.org for rel in rels] + is_all_org = False + orgs = [] + # Iterate over the taxonomyorgs instead of filtering to take advantage of prefetched data. + for taxonomy_org in taxonomy.taxonomyorg_set.all(): + if taxonomy_org.rel_type == rel_type: + if taxonomy_org.org is None: + is_all_org = True + else: + orgs.append(taxonomy_org.org) + + return (is_all_org, orgs) 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 723a90d8774a..e4fa403fa526 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/filters.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/filters.py @@ -6,7 +6,6 @@ from rest_framework.filters import BaseFilterBackend import openedx_tagging.core.tagging.rules as oel_tagging -from organizations.models import Organization from ...rules import get_admin_orgs, get_user_orgs from ...models import TaxonomyOrg @@ -25,9 +24,8 @@ def filter_queryset(self, request, queryset, _): if oel_tagging.is_taxonomy_admin(request.user): return queryset - orgs = list(Organization.objects.all()) - user_admin_orgs = get_admin_orgs(request.user, orgs) - user_orgs = get_user_orgs(request.user, orgs) # Orgs that the user is a content creator or instructor + user_admin_orgs = get_admin_orgs(request.user) + user_orgs = get_user_orgs(request.user) # Orgs that the user is a content creator or instructor if len(user_orgs) == 0 and len(user_admin_orgs) == 0: return queryset.none() @@ -67,11 +65,10 @@ 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') - orgs = list(Organization.objects.all()) - user_admin_orgs = get_admin_orgs(request.user, orgs) - user_orgs = get_user_orgs(request.user, orgs) + user_admin_orgs = get_admin_orgs(request.user) + user_orgs = get_user_orgs(request.user) user_or_admin_orgs = list(set(user_orgs) | set(user_admin_orgs)) return queryset.filter(taxonomy__enabled=True).filter( @@ -90,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/serializers.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py index 12433f8a381b..8bd26230855a 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py @@ -4,7 +4,6 @@ from __future__ import annotations -from django.core.exceptions import ObjectDoesNotExist from rest_framework import serializers, fields from openedx_tagging.core.tagging.rest_api.v1.serializers import ( @@ -14,26 +13,7 @@ from organizations.models import Organization - -class OptionalSlugRelatedField(serializers.SlugRelatedField): - """ - Modifies the DRF serializer SlugRelatedField. - - Non-existent slug values are represented internally as an empty queryset, instead of throwing a validation error. - """ - - def to_internal_value(self, data): - """ - Returns the object related to the given slug value, or an empty queryset if not found. - """ - - queryset = self.get_queryset() - try: - return queryset.get(**{self.slug_field: data}) - except ObjectDoesNotExist: - return queryset.none() - except (TypeError, ValueError): - self.fail('invalid') +from ...models import TaxonomyOrg class TaxonomyOrgListQueryParamsSerializer(TaxonomyListQueryParamsSerializer): @@ -41,13 +21,22 @@ class TaxonomyOrgListQueryParamsSerializer(TaxonomyListQueryParamsSerializer): Serializer for the query params for the GET view """ - org: fields.Field = OptionalSlugRelatedField( - slug_field="short_name", - queryset=Organization.objects.all(), + org: fields.Field = serializers.CharField( required=False, ) unassigned: fields.Field = serializers.BooleanField(required=False) + def validate(self, attrs: dict) -> dict: + """ + Validate the serializer data + """ + if "org" in attrs and "unassigned" in attrs: + raise serializers.ValidationError( + "'org' and 'unassigned' params cannot be both defined" + ) + + return attrs + class TaxonomyUpdateOrgBodySerializer(serializers.Serializer): """ @@ -86,14 +75,20 @@ class TaxonomyOrgSerializer(TaxonomySerializer): def get_orgs(self, obj) -> list[str]: """ Return the list of orgs for the taxonomy. - """ - return [taxonomy_org.org.short_name for taxonomy_org in obj.taxonomyorg_set.all() if taxonomy_org.org] + """ + return [ + taxonomy_org.org.short_name for taxonomy_org in obj.taxonomyorg_set.all() + if taxonomy_org.org and taxonomy_org.rel_type == TaxonomyOrg.RelType.OWNER + ] def get_all_orgs(self, obj) -> bool: """ Return True if the taxonomy is associated with all orgs. """ - return obj.taxonomyorg_set.filter(org__isnull=True).exists() + for taxonomy_org in obj.taxonomyorg_set.all(): + if taxonomy_org.org_id is None and taxonomy_org.rel_type == TaxonomyOrg.RelType.OWNER: + return True + return False class Meta: model = TaxonomySerializer.Meta.model 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 9b87f35e0483..eb5598c96a50 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 @@ -126,6 +126,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", @@ -498,27 +503,37 @@ def test_create_taxonomy(self, user_attr: str, expected_status: int) -> None: if user_attr == "staffA": assert response.data["orgs"] == [self.orgA.short_name] - def test_list_taxonomy_query_count(self): + @ddt.data( + ('staff', 11), + ("content_creatorA", 16), + ("library_staffA", 16), + ("library_userA", 16), + ("instructorA", 16), + ("course_instructorA", 16), + ("course_staffA", 16), + ) + @ddt.unpack + def test_list_taxonomy_query_count(self, user_attr: str, expected_queries: int): """ Test how many queries are used when retrieving taxonomies and permissions """ - url = TAXONOMY_ORG_LIST_URL + f'?org=${self.orgA.short_name}&enabled=true' - - self.client.force_authenticate(user=self.staff) - with self.assertNumQueries(16): # TODO Why so many queries? + url = TAXONOMY_ORG_LIST_URL + f'?org={self.orgA.short_name}&enabled=true' + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + with self.assertNumQueries(expected_queries): response = self.client.get(url) assert response.status_code == 200 - assert response.data["can_add_taxonomy"] - assert len(response.data["results"]) == 2 + assert response.data["can_add_taxonomy"] == user.is_staff + assert len(response.data["results"]) == 4 for taxonomy in response.data["results"]: if taxonomy["system_defined"]: assert not taxonomy["can_change_taxonomy"] assert not taxonomy["can_delete_taxonomy"] assert taxonomy["can_tag_object"] else: - assert taxonomy["can_change_taxonomy"] - assert taxonomy["can_delete_taxonomy"] + assert taxonomy["can_change_taxonomy"] == user.is_staff + assert taxonomy["can_delete_taxonomy"] == user.is_staff assert taxonomy["can_tag_object"] @@ -753,7 +768,7 @@ def test_detail_taxonomy_other_dont_see_no_org(self, user_attr: str) -> None: user_attr=user_attr, taxonomy_attr="ot1", expected_status=status.HTTP_404_NOT_FOUND, - reason="Only staff should see taxonomies with no org", + reason="Only taxonomy admins should see taxonomies with no org", ) @ddt.data( @@ -1232,11 +1247,12 @@ def test_update_org_no_perm(self, user_attr: str) -> None: self.client.force_authenticate(user=user) response = self.client.put(url, {"orgs": []}, format="json") - assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.status_code in [status.HTTP_403_FORBIDDEN, status.HTTP_404_NOT_FOUND] # Check that the orgs didn't change url = TAXONOMY_ORG_DETAIL_URL.format(pk=self.tA1.pk) response = self.client.get(url) + assert response.status_code == status.HTTP_200_OK assert response.data["orgs"] == [self.orgA.short_name] def test_update_org_check_permissions_orgA(self) -> None: @@ -1642,14 +1658,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) @@ -1661,14 +1678,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) @@ -1760,26 +1778,43 @@ def test_get_tags(self): assert status.is_success(response3.status_code) assert response3.data[str(self.courseA)]["taxonomies"] == expected_tags - def test_object_tags_query_count(self): + @ddt.data( + ('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( + self, + user_attr: str, + object_attr: str, + expected_queries: int, + expected_perm: bool = True): """ Test how many queries are used when retrieving object tags and permissions """ - object_key = self.courseA + object_key = getattr(self, object_attr) object_id = str(object_key) tagging_api.tag_object(object_id=object_id, taxonomy=self.t1, tags=["anvil", "android"]) expected_tags = [ - {"value": "android", "lineage": ["ALPHABET", "android"], "can_delete_objecttag": True}, - {"value": "anvil", "lineage": ["ALPHABET", "anvil"], "can_delete_objecttag": True}, + {"value": "android", "lineage": ["ALPHABET", "android"], "can_delete_objecttag": expected_perm}, + {"value": "anvil", "lineage": ["ALPHABET", "anvil"], "can_delete_objecttag": expected_perm}, ] - url = OBJECT_TAGS_URL.format(object_id=object_id) - self.client.force_authenticate(user=self.staff) - with self.assertNumQueries(7): # TODO Why so many queries? + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + with self.assertNumQueries(expected_queries): response = self.client.get(url) assert response.status_code == 200 assert len(response.data[object_id]["taxonomies"]) == 1 - assert response.data[object_id]["taxonomies"][0]["can_tag_object"] + assert response.data[object_id]["taxonomies"][0]["can_tag_object"] == expected_perm assert response.data[object_id]["taxonomies"][0]["tags"] == expected_tags @@ -2364,19 +2399,30 @@ class TestTaxonomyTagsViewSet(TestTaxonomyObjectsMixin, APITestCase): """ Test cases for TaxonomyTagsViewSet retrive action. """ - def test_taxonomy_tags_query_count(self): + @ddt.data( + ('staff', 11), + ("content_creatorA", 13), + ("library_staffA", 13), + ("library_userA", 13), + ("instructorA", 13), + ("course_instructorA", 13), + ("course_staffA", 13), + ) + @ddt.unpack + def test_taxonomy_tags_query_count(self, user_attr: str, expected_queries: int): """ Test how many queries are used when retrieving small taxonomies+tags and permissions """ url = f"{TAXONOMY_TAGS_URL}?search_term=an&parent_tag=ALPHABET".format(pk=self.t1.id) - self.client.force_authenticate(user=self.staff) - with self.assertNumQueries(13): # TODO Why so many queries? + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + with self.assertNumQueries(expected_queries): response = self.client.get(url) assert response.status_code == status.HTTP_200_OK - assert response.data["can_add_tag"] + assert response.data["can_add_tag"] == user.is_staff assert len(response.data["results"]) == 2 for taxonomy in response.data["results"]: - assert taxonomy["can_change_tag"] - assert taxonomy["can_delete_tag"] + assert taxonomy["can_change_tag"] == user.is_staff + assert taxonomy["can_delete_tag"] == user.is_staff 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 ca8343312bfd..cea834b38d04 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -6,6 +6,7 @@ import csv from typing import Iterator +from django.db.models import Count from django.http import StreamingHttpResponse from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey @@ -67,13 +68,8 @@ def get_queryset(self): query_params = TaxonomyOrgListQueryParamsSerializer(data=self.request.query_params.dict()) query_params.is_valid(raise_exception=True) enabled = query_params.validated_data.get("enabled", None) - unassigned = query_params.validated_data.get("unassigned", None) org = query_params.validated_data.get("org", None) - # Raise an error if both "org" and "unassigned" query params were provided - if "org" in query_params.validated_data and "unassigned" in query_params.validated_data: - raise ValidationError("'org' and 'unassigned' params cannot be both defined") - # If org filtering was requested, then use it, even if the org is invalid/None if "org" in query_params.validated_data: queryset = get_taxonomies_for_org(enabled, org) @@ -82,7 +78,13 @@ def get_queryset(self): else: queryset = get_taxonomies(enabled) - return queryset.prefetch_related("taxonomyorg_set") + # Prefetch taxonomyorgs so we can check permissions + queryset = queryset.prefetch_related("taxonomyorg_set__org") + + # Annotate with tags_count to avoid selecting all the tags + queryset = queryset.annotate(tags_count=Count("tag", distinct=True)) + + return queryset def perform_create(self, serializer): """ diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index fef71eeaf5de..a89e3618d715 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -7,11 +7,9 @@ import django.contrib.auth.models import openedx_tagging.core.tagging.rules as oel_tagging import rules -from django.db.models import Q from organizations.models import Organization from common.djangoapps.student.auth import has_studio_read_access, has_studio_write_access -from common.djangoapps.student.models import CourseAccessRole from common.djangoapps.student.roles import ( CourseInstructorRole, CourseStaffRole, @@ -20,11 +18,12 @@ OrgLibraryUserRole, OrgStaffRole ) -from openedx.core.djangoapps.content_libraries.api import get_libraries_for_user from .models import TaxonomyOrg -from .utils import get_context_key_from_key_string +from .utils import get_context_key_from_key_string, TaggingRulesCache + +rules_cache = TaggingRulesCache() UserType = Union[django.contrib.auth.models.User, django.contrib.auth.models.AnonymousUser] @@ -32,7 +31,6 @@ def is_org_admin(user: UserType, orgs: list[Organization] | None = None) -> bool """ Return True if the given user is an admin for any of the given orgs. """ - return len(get_admin_orgs(user, orgs)) > 0 @@ -45,19 +43,19 @@ def is_org_user(user: UserType, orgs: list[Organization]) -> bool: def get_admin_orgs(user: UserType, orgs: list[Organization] | None = None) -> list[Organization]: """ - Returns a list of orgs that the given user is an admin, from the given list of orgs. + Returns a list of orgs that the given user is an org-level staff, from the given list of orgs. If no orgs are provided, check all orgs """ - org_list = Organization.objects.all() if orgs is None else orgs + org_list = rules_cache.get_orgs() if orgs is None else orgs return [ org for org in org_list if OrgStaffRole(org=org.short_name).has_user(user) ] -def get_content_creator_orgs(user: UserType, orgs: list[Organization]) -> list[Organization]: +def _get_content_creator_orgs(user: UserType, orgs: list[Organization]) -> list[Organization]: """ - Returns a list of orgs that the given user is a content creator, the given list of orgs. + Returns a list of orgs that the given user is an org-level library user or instructor, from the given list of orgs. """ return [ org for org in orgs if ( @@ -68,43 +66,69 @@ def get_content_creator_orgs(user: UserType, orgs: list[Organization]) -> list[O ] -def get_instructor_orgs(user: UserType, orgs: list[Organization]) -> list[Organization]: - """ - Returns a list of orgs that the given user is an instructor, from the given list of orgs. +def _get_course_user_orgs(user: UserType, orgs: list[Organization]) -> list[Organization]: """ - instructor_roles = CourseAccessRole.objects.filter( - org__in=(org.short_name for org in orgs), - user=user, - role__in=(CourseStaffRole.ROLE, CourseInstructorRole.ROLE), - ) - instructor_orgs = [role.org for role in instructor_roles] - return [org for org in orgs if org.short_name in instructor_orgs] - + Returns a list of orgs for courses where the given user is staff or instructor, from the given list of orgs. -def get_library_user_orgs(user: UserType, orgs: list[Organization]) -> list[Organization]: - """ - Returns a list of orgs that the given user has explicity permission, from the given list of orgs. + Note: The user does not have org-level access to these orgs, only course-level access. So when checking ObjectTag + permissions, ensure that the user has staff/instructor access to the course/library with that object_id. """ + if not orgs: + return [] + + def user_has_role_ignore_course_id(user, role_name, org_name) -> bool: + """ + Returns True if the given user has the given role for the given org, OR for any courses in this org. + """ + # We use the user's RolesCache here to avoid re-querying. + # This cache gets populated the first time the user's permissions are checked (i.e when + # _get_content_creator_orgs is called). + + # pylint: disable=protected-access + roles_cache = user._roles + assert roles_cache + return any( + access_role.role in roles_cache.get_roles(role_name) and + access_role.org == org_name + for access_role in roles_cache._roles + ) + return [ org for org in orgs if ( - len(get_libraries_for_user(user, org=org.short_name)) > 0 + user_has_role_ignore_course_id(user, CourseStaffRole.ROLE, org.short_name) or + user_has_role_ignore_course_id(user, CourseInstructorRole.ROLE, org.short_name) ) ] -def get_user_orgs(user: UserType, orgs: list[Organization]) -> list[Organization]: +def _get_library_user_orgs(user: UserType, orgs: list[Organization]) -> list[Organization]: + """ + Returns a list of orgs (from the given list of orgs) that are associated with libraries that the given user has + explicitly been granted access to. + + Note: If no libraries exist for the given orgs, then no orgs will be returned, even though the user may be permitted + to access future libraries created in these orgs. + Nor does this mean the user may access all libraries in this org: library permissions are granted per library. + """ + library_orgs = rules_cache.get_library_orgs(user, [org.short_name for org in orgs]) + return list(set(library_orgs).intersection(orgs)) + + +def get_user_orgs(user: UserType, orgs: list[Organization] | None = None) -> list[Organization]: """ Return a list of orgs that the given user is a member of (instructor or content creator), from the given list of orgs. """ - content_creator_orgs = get_content_creator_orgs(user, orgs) - instructor_orgs = get_instructor_orgs(user, orgs) - library_user_orgs = get_library_user_orgs(user, orgs) - user_orgs = list(set(content_creator_orgs) | set(instructor_orgs) | set(library_user_orgs)) + org_list = rules_cache.get_orgs() if orgs is None else orgs + content_creator_orgs = _get_content_creator_orgs(user, org_list) + course_user_orgs = _get_course_user_orgs(user, org_list) + library_user_orgs = _get_library_user_orgs(user, org_list) + user_orgs = list(set(content_creator_orgs) | set(course_user_orgs) | set(library_user_orgs)) return user_orgs +@rules.predicate def can_create_taxonomy(user: UserType) -> bool: """ Returns True if the given user can create a taxonomy. @@ -141,21 +165,12 @@ def can_view_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: if oel_tagging.is_taxonomy_admin(user): return True - is_all_org = TaxonomyOrg.objects.filter( - taxonomy=taxonomy, - org=None, - rel_type=TaxonomyOrg.RelType.OWNER, - ).exists() + is_all_org, taxonomy_orgs = TaxonomyOrg.get_organizations(taxonomy) - # Enabled all-org taxonomies can be viewed by any registred user + # Enabled all-org taxonomies can be viewed by any registered user if is_all_org: return taxonomy.enabled - taxonomy_orgs = TaxonomyOrg.get_organizations( - taxonomy=taxonomy, - rel_type=TaxonomyOrg.RelType.OWNER, - ) - # Org-level staff can view any taxonomy that is associated with one of their orgs. if is_org_admin(user, taxonomy_orgs): return True @@ -191,21 +206,12 @@ def can_change_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: if oel_tagging.is_taxonomy_admin(user): return True - is_all_org = TaxonomyOrg.objects.filter( - taxonomy=taxonomy, - org=None, - rel_type=TaxonomyOrg.RelType.OWNER, - ).exists() + is_all_org, taxonomy_orgs = TaxonomyOrg.get_organizations(taxonomy) # Only taxonomy admins can edit all org taxonomies if is_all_org: return False - taxonomy_orgs = TaxonomyOrg.get_organizations( - taxonomy=taxonomy, - rel_type=TaxonomyOrg.RelType.OWNER, - ) - # Org-level staff can edit any taxonomy that is associated with one of their orgs. if is_org_admin(user, taxonomy_orgs): return True @@ -223,10 +229,15 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: try: context_key = get_context_key_from_key_string(object_id) - except ValueError: + assert context_key.org + except (ValueError, AssertionError): return False - return has_studio_write_access(user, context_key) + if has_studio_write_access(user, context_key): + return True + + object_org = rules_cache.get_orgs([context_key.org]) + return bool(object_org) and is_org_admin(user, object_org) @rules.predicate @@ -245,17 +256,25 @@ def can_view_object_tag_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) @rules.predicate def can_view_object_tag_objectid(user: UserType, object_id: str) -> bool: """ - Everyone that has permission to view the object should be able to tag it. + Everyone that has permission to view the object should be able to view its tags. """ if not object_id: raise ValueError("object_id must be provided") + if not user.is_authenticated: + return False + try: context_key = get_context_key_from_key_string(object_id) - except ValueError: + assert context_key.org + except (ValueError, AssertionError): return False - return has_studio_read_access(user, context_key) + if has_studio_read_access(user, context_key): + return True + + object_org = rules_cache.get_orgs([context_key.org]) + return bool(object_org) and (is_org_admin(user, object_org) or is_org_user(user, object_org)) @rules.predicate @@ -263,32 +282,28 @@ def can_change_object_tag( user: UserType, perm_obj: oel_tagging.ObjectTagPermissionItem | None = None ) -> bool: """ - Checks if the user has permissions to create or modify tags on the given taxonomy and object_id. - """ - if not oel_tagging.can_change_object_tag(user, perm_obj): - return False - - # The following code allows METHOD permission (PUT) in the viewset for everyone - if perm_obj is None: - return True + Returns True if the given user may change object tags with the given taxonomy + object_id. - # TaxonomySerializer use this rule passing object_id = "" to check if the user - # can use the taxonomy - if perm_obj.object_id == "": - return True + 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 - # Also skip taxonomy check if the taxonomy is not set - if not perm_obj.taxonomy: return True - - # Taxonomy admins can tag any object using any taxonomy - if oel_tagging.is_taxonomy_admin(user): - return True - - context_key = get_context_key_from_key_string(perm_obj.object_id) - - org_short_name = context_key.org - return perm_obj.taxonomy.taxonomyorg_set.filter(Q(org__short_name=org_short_name) | Q(org=None)).exists() + return False @rules.predicate @@ -324,7 +339,6 @@ def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None) 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.view_objecttag", oel_tagging.can_view_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 diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index 37d40ad61c24..229b8d62bc6a 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -157,11 +157,11 @@ def test_get_taxonomies_enabled_subclasses(self): ) @ddt.unpack def test_get_taxonomies_for_org(self, org_attr, enabled, expected): - org_owner = getattr(self, org_attr) if org_attr else None + org_owner = getattr(self, org_attr).short_name if org_attr else None taxonomies = list( taxonomy.cast() for taxonomy in api.get_taxonomies_for_org( - org_owner=org_owner, enabled=enabled + org_short_name=org_owner, enabled=enabled ) ) assert taxonomies == [ diff --git a/openedx/core/djangoapps/content_tagging/tests/test_rules.py b/openedx/core/djangoapps/content_tagging/tests/test_rules.py index 8dd8db125843..d53256f510a4 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) @@ -546,10 +546,11 @@ def test_object_tag_no_orgs(self, perm, tag_attr): "oel_tagging.add_objecttag", "oel_tagging.change_objecttag", "oel_tagging.delete_objecttag", + "oel_tagging.can_tag_object", ) def test_change_object_tag_all_orgs(self, perm): """ - Taxonomy administrators can create/edit an ObjectTag using taxonomies in their org, + Taxonomy administrators and org authors can create/edit an ObjectTag using taxonomies in their org, but only on objects they have write access to. """ for perm_item in self.all_org_perms: @@ -588,7 +589,7 @@ def test_change_object_tag_org1(self, perm, tag_attr): "tax_both_xblock2", ) def test_view_object_tag(self, tag_attr): - """Anyone can view any ObjectTag""" + """Content authors can view ObjectTags associated with enabled taxonomies in their org.""" perm = "oel_tagging.view_objecttag" perm_item = getattr(self, tag_attr) assert self.superuser.has_perm(perm, perm_item) diff --git a/openedx/core/djangoapps/content_tagging/utils.py b/openedx/core/djangoapps/content_tagging/utils.py index 7e8efa9a8933..3d7c340162da 100644 --- a/openedx/core/djangoapps/content_tagging/utils.py +++ b/openedx/core/djangoapps/content_tagging/utils.py @@ -3,9 +3,13 @@ """ from __future__ import annotations +from edx_django_utils.cache import RequestCache from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys.edx.locator import LibraryLocatorV2 +from organizations.models import Organization + +from openedx.core.djangoapps.content_libraries.api import get_libraries_for_user from .types import ContentKey @@ -42,3 +46,57 @@ def get_context_key_from_key_string(key_str: str) -> CourseKey | LibraryLocatorV return context_key raise ValueError("context must be a CourseKey or a LibraryLocatorV2") + + +class TaggingRulesCache: + """ + Caches data required for computing rules for the duration of the request. + """ + + def __init__(self): + """ + Initializes the request cache. + """ + self.request_cache = RequestCache('openedx.core.djangoapps.content_tagging.rules') + + def get_orgs(self, org_names: list[str] | None = None) -> list[Organization]: + """ + Returns the Organizations with the given name(s), or all Organizations if no names given. + + Organization instances are cached for the duration of the request. + """ + cache_key = 'all_orgs' + all_orgs = self.request_cache.data.get(cache_key) + if all_orgs is None: + all_orgs = { + org.short_name: org + for org in Organization.objects.all() + } + self.request_cache.set(cache_key, all_orgs) + + if org_names: + return [ + all_orgs[org_name] for org_name in org_names if org_name in all_orgs + ] + + return all_orgs.values() + + def get_library_orgs(self, user, org_names: list[str]) -> list[Organization]: + """ + Returns the Organizations that are associated with libraries that the given user has explicitly been granted + access to. + + These library orgs are cached for the duration of the request. + """ + cache_key = f'library_orgs:{user.id}' + library_orgs = self.request_cache.data.get(cache_key) + if library_orgs is None: + library_orgs = { + library.org.short_name: library.org + for library in get_libraries_for_user(user).select_related('org').only('org') + } + self.request_cache.set(cache_key, library_orgs) + + return [ + library_orgs[org_name] for org_name in org_names if org_name in library_orgs + ] diff --git a/requirements/constraints.txt b/requirements/constraints.txt index f08a2d3bf06e..72d6512f1cf1 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -108,7 +108,7 @@ libsass==0.10.0 click==8.1.6 # pinning this version to avoid updates while the library is being developed -openedx-learning==0.5.1 +openedx-learning==0.6.2 # Open AI version 1.0.0 dropped support for openai.ChatCompletion which is currently in use in enterprise. openai<=0.28.1 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 0a74f59aa7c9..347c910f4207 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -781,7 +781,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/kernel.in # lti-consumer-xblock -openedx-learning==0.5.1 +openedx-learning==0.6.2 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 1901fcbd7e43..79b5c652ecc8 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1309,7 +1309,7 @@ openedx-filters==1.6.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # lti-consumer-xblock -openedx-learning==0.5.1 +openedx-learning==0.6.2 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 7e288539d4ea..71efe3937ffd 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -921,7 +921,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.5.1 +openedx-learning==0.6.2 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 47b9be3f82fd..3ea36afa4398 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -979,7 +979,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.5.1 +openedx-learning==0.6.2 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt