From 0a3ada32cc408af63d8a3e7fdc69558e71dd0c6e Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 7 Feb 2024 13:19:37 +1030 Subject: [PATCH 01/22] test: fixes "list taxonomies for org" test URL Was requesting org=$orgA instead of org=orgA, and so wasn't testing the queries we needed to test. --- .../content_tagging/rest_api/v1/tests/test_views.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 20b1deb661b7..7dc101966fa4 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 @@ -491,15 +491,15 @@ def test_list_taxonomy_query_count(self): """ Test how many queries are used when retrieving taxonomies and permissions """ - url = TAXONOMY_ORG_LIST_URL + f'?org=${self.orgA.short_name}&enabled=true' + 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? + with self.assertNumQueries(23): # TODO Why so many queries? response = self.client.get(url) assert response.status_code == 200 assert response.data["can_add_taxonomy"] - assert len(response.data["results"]) == 2 + assert len(response.data["results"]) == 4 for taxonomy in response.data["results"]: if taxonomy["system_defined"]: assert not taxonomy["can_change_taxonomy"] From 8e7823eb344e9d8a5c561e33c2c642f0cbc453df Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 7 Feb 2024 13:25:52 +1030 Subject: [PATCH 02/22] fix: filter list of taxonomies by org without fetching the Organization --- .../core/djangoapps/content_tagging/api.py | 3 +- .../rest_api/v1/serializers.py | 36 +++++++------------ .../rest_api/v1/tests/test_views.py | 2 +- .../content_tagging/rest_api/v1/views.py | 5 --- .../content_tagging/tests/test_api.py | 4 +-- 5 files changed, 16 insertions(+), 34 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 70aa7e2150da..3db70f96b4d4 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -82,7 +82,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. @@ -95,7 +95,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/rest_api/v1/serializers.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py index 12433f8a381b..2f526dedc301 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py @@ -15,39 +15,27 @@ 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') - - 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): """ 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 7dc101966fa4..855a34a3f926 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 @@ -494,7 +494,7 @@ def test_list_taxonomy_query_count(self): url = TAXONOMY_ORG_LIST_URL + f'?org={self.orgA.short_name}&enabled=true' self.client.force_authenticate(user=self.staff) - with self.assertNumQueries(23): # TODO Why so many queries? + with self.assertNumQueries(21): # TODO Why so many queries? response = self.client.get(url) assert response.status_code == 200 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 151bc09f5d76..fff2404aa95b 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -56,13 +56,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) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index 9a297be968b1..837e17e7c19c 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -145,11 +145,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 == [ From 3057ccf89634dc1141287b703d238c0d7d821503 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 7 Feb 2024 15:26:47 +1030 Subject: [PATCH 03/22] fix: prefetch tag_set to reduce queries --- .../djangoapps/content_tagging/rest_api/v1/tests/test_views.py | 2 +- openedx/core/djangoapps/content_tagging/rest_api/v1/views.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) 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 855a34a3f926..1e306e1a42b9 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 @@ -494,7 +494,7 @@ def test_list_taxonomy_query_count(self): url = TAXONOMY_ORG_LIST_URL + f'?org={self.orgA.short_name}&enabled=true' self.client.force_authenticate(user=self.staff) - with self.assertNumQueries(21): # TODO Why so many queries? + with self.assertNumQueries(19): # TODO Why so many queries? response = self.client.get(url) assert response.status_code == 200 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 fff2404aa95b..1b6c22a8fbe1 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -66,7 +66,8 @@ def get_queryset(self): else: queryset = get_taxonomies(enabled) - return queryset.prefetch_related("taxonomyorg_set") + # Prefetch tag_set so we can serialize the tag counts + return queryset.prefetch_related("taxonomyorg_set", "tag_set") def perform_create(self, serializer): """ From 8a0d598d5ffe1993d17236833b68a250d66400e8 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 7 Feb 2024 16:36:46 +1030 Subject: [PATCH 04/22] fix: lint --- .../core/djangoapps/content_tagging/rest_api/v1/serializers.py | 1 - openedx/core/djangoapps/content_tagging/rest_api/v1/views.py | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) 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 2f526dedc301..43c0fbff9517 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 ( 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 1b6c22a8fbe1..11bbea265184 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -5,7 +5,7 @@ from openedx_tagging.core.tagging.rest_api.v1.views import ObjectTagView, TaxonomyView from rest_framework import status from rest_framework.decorators import action -from rest_framework.exceptions import PermissionDenied, ValidationError +from rest_framework.exceptions import PermissionDenied from rest_framework.request import Request from rest_framework.response import Response From 6e094ef3e380fa1084c368da57da4ca25169a636 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 8 Feb 2024 02:56:16 +1030 Subject: [PATCH 05/22] fix: prefetch taxonomyorg orgs --- .../djangoapps/content_tagging/rest_api/v1/tests/test_views.py | 2 +- openedx/core/djangoapps/content_tagging/rest_api/v1/views.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 1e306e1a42b9..bb6e5d4930cd 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 @@ -494,7 +494,7 @@ def test_list_taxonomy_query_count(self): url = TAXONOMY_ORG_LIST_URL + f'?org={self.orgA.short_name}&enabled=true' self.client.force_authenticate(user=self.staff) - with self.assertNumQueries(19): # TODO Why so many queries? + with self.assertNumQueries(17): # TODO Why so many queries? response = self.client.get(url) assert response.status_code == 200 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 11bbea265184..a1a1a318014d 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -67,7 +67,7 @@ def get_queryset(self): queryset = get_taxonomies(enabled) # Prefetch tag_set so we can serialize the tag counts - return queryset.prefetch_related("taxonomyorg_set", "tag_set") + return queryset.prefetch_related("taxonomyorg_set__org", "tag_set") def perform_create(self, serializer): """ From 016f2a54351257a66d0c90abe2e7079e7940ccb9 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 8 Feb 2024 10:04:52 +1030 Subject: [PATCH 06/22] temp: use openedx-learning branch from pr#157 which reduces the query count. TO DO: once PR is merged and tagged, update the requirement here. --- .../djangoapps/content_tagging/rest_api/v1/tests/test_views.py | 2 +- requirements/constraints.txt | 2 +- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) 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 07e329c6c17f..d4695b488520 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 @@ -504,7 +504,7 @@ def test_list_taxonomy_query_count(self): url = TAXONOMY_ORG_LIST_URL + f'?org={self.orgA.short_name}&enabled=true' self.client.force_authenticate(user=self.staff) - with self.assertNumQueries(17): # TODO Why so many queries? + with self.assertNumQueries(15): # TODO Why so many queries? response = self.client.get(url) assert response.status_code == 200 diff --git a/requirements/constraints.txt b/requirements/constraints.txt index bd78dbb0d65c..7dea165ac062 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 @ git+https://github.com/open-craft/openedx-learning.git@jill/tagging-less-queries # 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 7d27ac9978ec..69d774237928 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -782,7 +782,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/kernel.in # lti-consumer-xblock -openedx-learning==0.5.1 +openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/tagging-less-queries # via # -c requirements/edx/../constraints.txt # -r requirements/edx/kernel.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index bfe3fbd2b77e..07c51dd6700b 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1313,7 +1313,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 @ git+https://github.com/open-craft/openedx-learning.git@jill/tagging-less-queries # via # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 92d84425d0af..4acf19eb652b 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -924,7 +924,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.5.1 +openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/tagging-less-queries # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index eae1dbdfc33a..e645cbfbb0da 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -982,7 +982,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.5.1 +openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/tagging-less-queries # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt From 38bef63819bb60d2e1d3f25b4e5e6e9151af3fdb Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 8 Feb 2024 10:23:31 +1030 Subject: [PATCH 07/22] fix: iterate over prefetched taxonomyorgs instead of filtering to avoid re-querying. Changes TaxonomyOrg.get_organizations to return the "all orgs" flag too, which makes it useful in the rules. --- .../djangoapps/content_tagging/models/base.py | 27 +++++++++++-------- .../rest_api/v1/serializers.py | 6 ++++- .../rest_api/v1/tests/test_views.py | 2 +- .../core/djangoapps/content_tagging/rules.py | 22 ++------------- 4 files changed, 24 insertions(+), 33 deletions(-) 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/serializers.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py index 43c0fbff9517..7edcd22c2ecf 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py @@ -80,7 +80,11 @@ 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() + is_all_orgs = False + for taxonomy_org in obj.taxonomyorg_set.all(): + if taxonomy_org.org_id is None: + 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 d4695b488520..228aedf9130e 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 @@ -504,7 +504,7 @@ def test_list_taxonomy_query_count(self): url = TAXONOMY_ORG_LIST_URL + f'?org={self.orgA.short_name}&enabled=true' self.client.force_authenticate(user=self.staff) - with self.assertNumQueries(15): # TODO Why so many queries? + with self.assertNumQueries(11): response = self.client.get(url) assert response.status_code == 200 diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index fef71eeaf5de..ad4f1fa57ac0 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -141,21 +141,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 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 +182,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 From e5819519f9b7634ceb592f308b9c7cd0cce25158 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 8 Feb 2024 10:24:42 +1030 Subject: [PATCH 08/22] fix: Adds a check for the TaxonomyOrg.rel_type when serializing orgs --- .../content_tagging/rest_api/v1/serializers.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) 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 7edcd22c2ecf..3e4d1a047a10 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py @@ -13,6 +13,8 @@ from organizations.models import Organization +from ...models import TaxonomyOrg + class TaxonomyOrgListQueryParamsSerializer(TaxonomyListQueryParamsSerializer): """ @@ -73,8 +75,11 @@ 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: """ @@ -82,7 +87,7 @@ def get_all_orgs(self, obj) -> bool: """ is_all_orgs = False for taxonomy_org in obj.taxonomyorg_set.all(): - if taxonomy_org.org_id is None: + if taxonomy_org.org_id is None and taxonomy_org.rel_type == TaxonomyOrg.RelType.OWNER: return True return False From b46f466d3d1d459e4ed1523fb857cb21269de493 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 8 Feb 2024 19:30:59 +1030 Subject: [PATCH 09/22] fix: oel_tagging library change reduced query count for the taxonomy tags views. --- .../content_tagging/rest_api/v1/tests/test_views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 228aedf9130e..fee992cea559 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 @@ -1773,7 +1773,7 @@ def test_object_tags_query_count(self): 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? + with self.assertNumQueries(7): response = self.client.get(url) assert response.status_code == 200 @@ -2299,7 +2299,7 @@ def test_taxonomy_tags_query_count(self): 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? + with self.assertNumQueries(11): response = self.client.get(url) assert response.status_code == status.HTTP_200_OK From 5dbae2f58c2b9bb24dcf6ec896bc22280333fcae Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 8 Feb 2024 20:07:16 +1030 Subject: [PATCH 10/22] fix: annotate taxonomies with their tags_count instead of prefetching all the tags. --- .../djangoapps/content_tagging/rest_api/v1/views.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) 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 a1a1a318014d..78547b8f74b5 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -1,6 +1,7 @@ """ Tagging Org API Views """ +from django.db.models import Count from openedx_tagging.core.tagging import rules as oel_tagging_rules from openedx_tagging.core.tagging.rest_api.v1.views import ObjectTagView, TaxonomyView from rest_framework import status @@ -18,8 +19,8 @@ set_taxonomy_orgs, ) from ...rules import get_admin_orgs -from .serializers import TaxonomyOrgListQueryParamsSerializer, TaxonomyOrgSerializer, TaxonomyUpdateOrgBodySerializer from .filters import ObjectTagTaxonomyOrgFilterBackend, UserOrgFilterBackend +from .serializers import TaxonomyOrgListQueryParamsSerializer, TaxonomyOrgSerializer, TaxonomyUpdateOrgBodySerializer class TaxonomyOrgView(TaxonomyView): @@ -66,8 +67,13 @@ def get_queryset(self): else: queryset = get_taxonomies(enabled) - # Prefetch tag_set so we can serialize the tag counts - return queryset.prefetch_related("taxonomyorg_set__org", "tag_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): """ From 392ccf2240f5f6972e6e15618f4b130c9bf0fb54 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 8 Feb 2024 20:37:11 +1030 Subject: [PATCH 11/22] test: adds query count tests for non-taxonomy admins (needs fixing) --- .../rest_api/v1/tests/test_views.py | 65 ++++++++++++++----- 1 file changed, 48 insertions(+), 17 deletions(-) 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 fee992cea559..6ac707a27169 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 @@ -497,18 +497,28 @@ 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", 25), # FIXME too many queries. + ("library_staffA", 25), + ("library_userA", 25), + ("instructorA", 25), + ("course_instructorA", 25), + ("course_staffA", 25), + ) + @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(11): + 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 response.data["can_add_taxonomy"] == user.is_staff assert len(response.data["results"]) == 4 for taxonomy in response.data["results"]: if taxonomy["system_defined"]: @@ -516,8 +526,8 @@ def test_list_taxonomy_query_count(self): 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"] @@ -1759,7 +1769,17 @@ 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', 7), + #("content_creatorA", 8), # FIXME 403? + #("library_staffA", 8), + #("library_userA", 8), + ("instructorA", 19), # FIXME too many queries. + ("course_instructorA", 19), + ("course_staffA", 19), + ) + @ddt.unpack + def test_object_tags_query_count(self, user_attr: str, expected_queries: int): """ Test how many queries are used when retrieving object tags and permissions """ @@ -1770,10 +1790,10 @@ def test_object_tags_query_count(self): {"value": "android", "lineage": ["ALPHABET", "android"], "can_delete_objecttag": True}, {"value": "anvil", "lineage": ["ALPHABET", "anvil"], "can_delete_objecttag": True}, ] - url = OBJECT_TAGS_URL.format(object_id=object_id) - self.client.force_authenticate(user=self.staff) - with self.assertNumQueries(7): + 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 @@ -2292,19 +2312,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), # FIXME too many queries? + ("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(11): + 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 From 07031d0830c40b1c79902a39c283ed6f61716d5f Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Mon, 12 Feb 2024 18:31:47 +1030 Subject: [PATCH 12/22] fix: use qs.exists() instead of len(qs) > 0 because SELECT COUNT at the database level is more efficient than returning a bunch of data we don't need. --- openedx/core/djangoapps/content_tagging/rules.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index ad4f1fa57ac0..4677f461f02c 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -83,12 +83,15 @@ def get_instructor_orgs(user: UserType, orgs: list[Organization]) -> list[Organi 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. + Returns a list of orgs (from the given list of orgs) that are associated with libraries that the given user has + explicitly been granted read access for. + + 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. """ return [ - org for org in orgs if ( - len(get_libraries_for_user(user, org=org.short_name)) > 0 - ) + org for org in orgs if get_libraries_for_user(user, org=org.short_name).exists() ] From ab01a7912b1f7afa99cf14c31656db1e4125d1e8 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 13 Feb 2024 01:11:00 +1030 Subject: [PATCH 13/22] fix: fetch all content library orgs at once --- .../core/djangoapps/content_libraries/api.py | 5 ++++- .../rest_api/v1/tests/test_views.py | 20 +++++++++---------- .../core/djangoapps/content_tagging/rules.py | 6 +++--- 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 26e7c652d983..60a0afd8c8c8 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -291,7 +291,10 @@ def get_libraries_for_user(user, org=None, library_type=None): """ filter_kwargs = {} if org: - filter_kwargs['org__short_name'] = org + if isinstance(org, list): + filter_kwargs['org__short_name__in'] = org + else: + filter_kwargs['org__short_name'] = org if library_type: filter_kwargs['type'] = library_type qs = ContentLibrary.objects.filter(**filter_kwargs) 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 6ac707a27169..42dfa282843d 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 @@ -499,12 +499,12 @@ def test_create_taxonomy(self, user_attr: str, expected_status: int) -> None: @ddt.data( ('staff', 11), - ("content_creatorA", 25), # FIXME too many queries. - ("library_staffA", 25), - ("library_userA", 25), - ("instructorA", 25), - ("course_instructorA", 25), - ("course_staffA", 25), + ("content_creatorA", 22), # FIXME too many queries. + ("library_staffA", 22), + ("library_userA", 22), + ("instructorA", 22), + ("course_instructorA", 22), + ("course_staffA", 22), ) @ddt.unpack def test_list_taxonomy_query_count(self, user_attr: str, expected_queries: int): @@ -762,7 +762,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( @@ -1774,9 +1774,9 @@ def test_get_tags(self): #("content_creatorA", 8), # FIXME 403? #("library_staffA", 8), #("library_userA", 8), - ("instructorA", 19), # FIXME too many queries. - ("course_instructorA", 19), - ("course_staffA", 19), + ("instructorA", 17), # FIXME too many queries. + ("course_instructorA", 17), + ("course_staffA", 17), ) @ddt.unpack def test_object_tags_query_count(self, user_attr: str, expected_queries: int): diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 4677f461f02c..06ece0fdd4f3 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -90,9 +90,9 @@ def get_library_user_orgs(user: UserType, orgs: list[Organization]) -> list[Orga 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. """ - return [ - org for org in orgs if get_libraries_for_user(user, org=org.short_name).exists() - ] + libraries = get_libraries_for_user(user, org=[org.short_name for org in orgs]).select_related('org').only('org') + library_orgs = [library.org for library in libraries] + return list(set(library_orgs).intersection(orgs)) def get_user_orgs(user: UserType, orgs: list[Organization]) -> list[Organization]: From 76b080befcea19cd2ced2f4432e20a94b29a8e3a Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 13 Feb 2024 03:39:32 +1030 Subject: [PATCH 14/22] fix: reduced query counts by using the user's RoleCache instead of querying for CourseAccessRoles Also * removes the can_change_objecttag override method -- we only need to override the can_change_objecttag_taxonomy / _object+id methods to achieve our rules changes. * fixes up some misleading comments, and underscores some internal rules method names. --- .../rest_api/v1/tests/test_views.py | 46 +++++--- .../core/djangoapps/content_tagging/rules.py | 110 ++++++++---------- .../content_tagging/tests/test_rules.py | 5 +- 3 files changed, 81 insertions(+), 80 deletions(-) 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 42dfa282843d..7fe1bf7b1fc3 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 @@ -499,12 +499,12 @@ def test_create_taxonomy(self, user_attr: str, expected_status: int) -> None: @ddt.data( ('staff', 11), - ("content_creatorA", 22), # FIXME too many queries. - ("library_staffA", 22), - ("library_userA", 22), - ("instructorA", 22), - ("course_instructorA", 22), - ("course_staffA", 22), + ("content_creatorA", 19), # FIXME too many queries. + ("library_staffA", 19), + ("library_userA", 19), + ("instructorA", 19), + ("course_instructorA", 19), + ("course_staffA", 19), ) @ddt.unpack def test_list_taxonomy_query_count(self, user_attr: str, expected_queries: int): @@ -1241,11 +1241,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: @@ -1770,25 +1771,32 @@ def test_get_tags(self): assert response3.data[str(self.courseA)]["taxonomies"] == expected_tags @ddt.data( - ('staff', 7), - #("content_creatorA", 8), # FIXME 403? - #("library_staffA", 8), - #("library_userA", 8), - ("instructorA", 17), # FIXME too many queries. - ("course_instructorA", 17), - ("course_staffA", 17), + ('staff', 'courseA', 7), + ('staff', 'libraryA', 7), + ("content_creatorA", 'courseA', 20, False), # FIXME too many queries. + ("content_creatorA", 'libraryA', 20, False), + ("library_staffA", 'libraryA', 20, False), # Library users can only view objecttags, not change them. + ("library_userA", 'libraryA', 20, False), + ("instructorA", 'courseA', 13), + ("course_instructorA", 'courseA', 13), + ("course_staffA", 'courseA', 13), ) @ddt.unpack - def test_object_tags_query_count(self, user_attr: str, expected_queries: int): + 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) user = getattr(self, user_attr) @@ -1798,7 +1806,7 @@ def test_object_tags_query_count(self, user_attr: str, expected_queries: int): 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 diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 06ece0fdd4f3..7cf75a979465 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, @@ -45,7 +43,7 @@ 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 """ @@ -55,9 +53,9 @@ def get_admin_orgs(user: UserType, orgs: list[Organization] | None = None) -> li ] -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,20 +66,42 @@ def get_content_creator_orgs(user: UserType, orgs: list[Organization]) -> list[O ] -def get_instructor_orgs(user: UserType, orgs: list[Organization]) -> list[Organization]: +def _get_course_user_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. + Returns a list of orgs for courses where the given user is staff or instructor, 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. """ - 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] + 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 ( + 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_library_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 read access for. @@ -100,14 +120,15 @@ def get_user_orgs(user: UserType, orgs: list[Organization]) -> 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)) + content_creator_orgs = _get_content_creator_orgs(user, orgs) + course_user_orgs = _get_course_user_orgs(user, orgs) + library_user_orgs = _get_library_user_orgs(user, orgs) + 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. @@ -146,7 +167,7 @@ def can_view_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: 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 @@ -211,7 +232,11 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: except ValueError: return False - return has_studio_write_access(user, context_key) + if has_studio_write_access(user, context_key): + return True + + object_org = Organization.objects.filter(short_name=context_key.org).first() + return object_org and is_org_admin(user, [object_org]) @rules.predicate @@ -230,7 +255,7 @@ 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") @@ -240,40 +265,11 @@ def can_view_object_tag_objectid(user: UserType, object_id: str) -> bool: except ValueError: return False - return has_studio_read_access(user, context_key) - - -@rules.predicate -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 - - # TaxonomySerializer use this rule passing object_id = "" to check if the user - # can use the taxonomy - if perm_obj.object_id == "": + if has_studio_read_access(user, context_key): return True - # 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() + object_org = Organization.objects.filter(short_name=context_key.org).first() + return object_org and (is_org_admin(user, [object_org]) or is_org_user(user, [object_org])) @rules.predicate @@ -306,11 +302,7 @@ 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.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) +rules.set_perm("oel_tagging.can_tag_object", oel_tagging.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 8dd8db125843..d64fd3449ea9 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_rules.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_rules.py @@ -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) From 85eb4f15d0c3bf456b28fbd427b300d250420018 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 13 Feb 2024 04:15:31 +1030 Subject: [PATCH 15/22] fix: adds content_tagging rules cache for Organization list Avoids fetching and re-fetching Organizations by caching the full Organization list for the duration of the request. --- .../content_tagging/rest_api/v1/filters.py | 11 +++--- .../rest_api/v1/tests/test_views.py | 20 +++++------ .../core/djangoapps/content_tagging/rules.py | 26 ++++++++------ .../core/djangoapps/content_tagging/utils.py | 36 +++++++++++++++++++ 4 files changed, 65 insertions(+), 28 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 723a90d8774a..cbf947612fc5 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() @@ -69,9 +67,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) + 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( 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 7fe1bf7b1fc3..4029772f2249 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 @@ -499,12 +499,12 @@ def test_create_taxonomy(self, user_attr: str, expected_status: int) -> None: @ddt.data( ('staff', 11), - ("content_creatorA", 19), # FIXME too many queries. - ("library_staffA", 19), - ("library_userA", 19), - ("instructorA", 19), - ("course_instructorA", 19), - ("course_staffA", 19), + ("content_creatorA", 18), # FIXME too many queries. + ("library_staffA", 18), + ("library_userA", 18), + ("instructorA", 18), + ("course_instructorA", 18), + ("course_staffA", 18), ) @ddt.unpack def test_list_taxonomy_query_count(self, user_attr: str, expected_queries: int): @@ -1773,10 +1773,10 @@ def test_get_tags(self): @ddt.data( ('staff', 'courseA', 7), ('staff', 'libraryA', 7), - ("content_creatorA", 'courseA', 20, False), # FIXME too many queries. - ("content_creatorA", 'libraryA', 20, False), - ("library_staffA", 'libraryA', 20, False), # Library users can only view objecttags, not change them. - ("library_userA", 'libraryA', 20, False), + ("content_creatorA", 'courseA', 15, False), # FIXME too many queries. + ("content_creatorA", 'libraryA', 15, False), + ("library_staffA", 'libraryA', 15, False), # Library users can only view objecttags, not change them. + ("library_userA", 'libraryA', 15, False), ("instructorA", 'courseA', 13), ("course_instructorA", 'courseA', 13), ("course_staffA", 'courseA', 13), diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 7cf75a979465..39afe7e12809 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -21,8 +21,10 @@ 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] @@ -30,7 +32,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 @@ -47,7 +48,7 @@ def get_admin_orgs(user: UserType, orgs: list[Organization] | None = None) -> li 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) ] @@ -115,14 +116,15 @@ def _get_library_user_orgs(user: UserType, orgs: list[Organization]) -> list[Org return list(set(library_orgs).intersection(orgs)) -def get_user_orgs(user: UserType, orgs: list[Organization]) -> list[Organization]: +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) - course_user_orgs = _get_course_user_orgs(user, orgs) - library_user_orgs = _get_library_user_orgs(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 @@ -235,8 +237,9 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: if has_studio_write_access(user, context_key): return True - object_org = Organization.objects.filter(short_name=context_key.org).first() - return object_org and is_org_admin(user, [object_org]) + assert context_key.org + object_org = rules_cache.get_orgs([context_key.org]) + return bool(object_org) and is_org_admin(user, object_org) @rules.predicate @@ -268,8 +271,9 @@ def can_view_object_tag_objectid(user: UserType, object_id: str) -> bool: if has_studio_read_access(user, context_key): return True - object_org = Organization.objects.filter(short_name=context_key.org).first() - return object_org and (is_org_admin(user, [object_org]) or is_org_user(user, [object_org])) + assert context_key.org + 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 diff --git a/openedx/core/djangoapps/content_tagging/utils.py b/openedx/core/djangoapps/content_tagging/utils.py index 7e8efa9a8933..42d6eee6bc8a 100644 --- a/openedx/core/djangoapps/content_tagging/utils.py +++ b/openedx/core/djangoapps/content_tagging/utils.py @@ -3,9 +3,11 @@ """ 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 .types import ContentKey @@ -42,3 +44,37 @@ 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() From 8ee319b7cc3aca670a5df60557eaa686082f06de Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 13 Feb 2024 04:59:50 +1030 Subject: [PATCH 16/22] fix: adds library orgs cache Avoids re-fetching the libraries that a user has been granted access to by caching the data we need from these libraries for the duration of the request. --- .../core/djangoapps/content_libraries/api.py | 5 +---- .../rest_api/v1/tests/test_views.py | 20 ++++++++--------- .../core/djangoapps/content_tagging/rules.py | 6 ++--- .../core/djangoapps/content_tagging/utils.py | 22 +++++++++++++++++++ 4 files changed, 35 insertions(+), 18 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 60a0afd8c8c8..26e7c652d983 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -291,10 +291,7 @@ def get_libraries_for_user(user, org=None, library_type=None): """ filter_kwargs = {} if org: - if isinstance(org, list): - filter_kwargs['org__short_name__in'] = org - else: - filter_kwargs['org__short_name'] = org + filter_kwargs['org__short_name'] = org if library_type: filter_kwargs['type'] = library_type qs = ContentLibrary.objects.filter(**filter_kwargs) 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 4029772f2249..7b3efd9b4fac 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 @@ -499,12 +499,12 @@ def test_create_taxonomy(self, user_attr: str, expected_status: int) -> None: @ddt.data( ('staff', 11), - ("content_creatorA", 18), # FIXME too many queries. - ("library_staffA", 18), - ("library_userA", 18), - ("instructorA", 18), - ("course_instructorA", 18), - ("course_staffA", 18), + ("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): @@ -1773,10 +1773,10 @@ def test_get_tags(self): @ddt.data( ('staff', 'courseA', 7), ('staff', 'libraryA', 7), - ("content_creatorA", 'courseA', 15, False), # FIXME too many queries. - ("content_creatorA", 'libraryA', 15, False), - ("library_staffA", 'libraryA', 15, False), # Library users can only view objecttags, not change them. - ("library_userA", 'libraryA', 15, False), + ("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), diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 39afe7e12809..67623bef22b9 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -18,7 +18,6 @@ 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, TaggingRulesCache @@ -105,14 +104,13 @@ def user_has_role_ignore_course_id(user, role_name, org_name) -> bool: 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 read access for. + 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. """ - libraries = get_libraries_for_user(user, org=[org.short_name for org in orgs]).select_related('org').only('org') - library_orgs = [library.org for library in libraries] + library_orgs = rules_cache.get_library_orgs(user, [org.short_name for org in orgs]) return list(set(library_orgs).intersection(orgs)) diff --git a/openedx/core/djangoapps/content_tagging/utils.py b/openedx/core/djangoapps/content_tagging/utils.py index 42d6eee6bc8a..3d7c340162da 100644 --- a/openedx/core/djangoapps/content_tagging/utils.py +++ b/openedx/core/djangoapps/content_tagging/utils.py @@ -9,6 +9,8 @@ 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 @@ -78,3 +80,23 @@ def get_orgs(self, org_names: list[str] | None = None) -> list[Organization]: ] 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 + ] From 05a6fd97f42f4ced65c50f6d1093bbf2f079fe43 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 15 Feb 2024 11:04:46 +1030 Subject: [PATCH 17/22] fix: address nits in PR review --- .../djangoapps/content_tagging/rest_api/v1/serializers.py | 1 - openedx/core/djangoapps/content_tagging/rules.py | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) 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 3e4d1a047a10..ba5d1690658c 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py @@ -85,7 +85,6 @@ def get_all_orgs(self, obj) -> bool: """ Return True if the taxonomy is associated with all orgs. """ - is_all_orgs = False 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 diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 67623bef22b9..e672b8790a50 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -229,13 +229,13 @@ 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 if has_studio_write_access(user, context_key): return True - assert context_key.org object_org = rules_cache.get_orgs([context_key.org]) return bool(object_org) and is_org_admin(user, object_org) @@ -263,13 +263,13 @@ def can_view_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 if has_studio_read_access(user, context_key): return True - assert context_key.org 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)) From 011a99bf1767e2fec1a429a9cb0b9938e1a776c2 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 15 Feb 2024 11:24:08 +1030 Subject: [PATCH 18/22] fix: nit --- .../core/djangoapps/content_tagging/rest_api/v1/serializers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 ba5d1690658c..8bd26230855a 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/serializers.py @@ -29,7 +29,7 @@ class TaxonomyOrgListQueryParamsSerializer(TaxonomyListQueryParamsSerializer): 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" From eac1f267d2c9db5264e82a2ba1a66003b8f62250 Mon Sep 17 00:00:00 2001 From: Jillian Date: Thu, 15 Feb 2024 12:10:17 +1030 Subject: [PATCH 19/22] 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. --- .../content_tagging/rest_api/v1/filters.py | 4 +-- .../rest_api/v1/tests/test_views.py | 35 +++++++++++-------- .../core/djangoapps/content_tagging/rules.py | 34 +++++++++++++++++- .../content_tagging/tests/test_rules.py | 2 +- 4 files changed, 57 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 cbf947612fc5..e4fa403fa526 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 7b3efd9b4fac..76974a547485 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 e672b8790a50..47638f2ebefa 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/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. diff --git a/openedx/core/djangoapps/content_tagging/tests/test_rules.py b/openedx/core/djangoapps/content_tagging/tests/test_rules.py index d64fd3449ea9..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) From 95861c72eed754515623c646bda9830c641d11dc Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 15 Feb 2024 14:04:34 +1030 Subject: [PATCH 20/22] feat: updates openedx-learning dependency --- requirements/constraints.txt | 2 +- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index 7dea165ac062..47f2f41c1347 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 @ git+https://github.com/open-craft/openedx-learning.git@jill/tagging-less-queries +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 69d774237928..84011f2ca09c 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -782,7 +782,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/kernel.in # lti-consumer-xblock -openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/tagging-less-queries +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 07c51dd6700b..aa100566d386 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1313,7 +1313,7 @@ openedx-filters==1.6.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # lti-consumer-xblock -openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/tagging-less-queries +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 4acf19eb652b..020854eb2a44 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -924,7 +924,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/tagging-less-queries +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 e645cbfbb0da..0d482f32b5bc 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -982,7 +982,7 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning @ git+https://github.com/open-craft/openedx-learning.git@jill/tagging-less-queries +openedx-learning==0.6.2 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt From 3b65f89afc083af6c085260623856615a0241f04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 16 Feb 2024 16:15:10 -0300 Subject: [PATCH 21/22] fix: missing import --- openedx/core/djangoapps/content_tagging/rest_api/v1/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 86d6f5ed632f..cea834b38d04 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/views.py @@ -14,7 +14,7 @@ from openedx_tagging.core.tagging.rest_api.v1.views import ObjectTagView, TaxonomyView from rest_framework import status from rest_framework.decorators import action -from rest_framework.exceptions import PermissionDenied +from rest_framework.exceptions import PermissionDenied, ValidationError from rest_framework.request import Request from rest_framework.response import Response from rest_framework.views import APIView From 029624119068c0d013f0f2d8d51602f56ab882d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 16 Feb 2024 16:36:00 -0300 Subject: [PATCH 22/22] fix: return False if user is anonymous --- openedx/core/djangoapps/content_tagging/rules.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 47638f2ebefa..a89e3618d715 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -261,6 +261,9 @@ def can_view_object_tag_objectid(user: UserType, object_id: str) -> bool: 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) assert context_key.org