From d72ae83f89679766be8a0bc887639d8670524ad0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 30 Jan 2024 21:41:44 -0300 Subject: [PATCH 01/12] refactor: remove ContentObjectTag model and related functions --- .../contentstore/views/component.py | 4 +- .../core/djangoapps/content_tagging/api.py | 51 +++++++++---------- .../0008_remove_content_object_tag.py | 16 ++++++ .../content_tagging/models/__init__.py | 1 - .../djangoapps/content_tagging/models/base.py | 35 +------------ .../rest_api/v1/tests/test_views.py | 2 +- .../core/djangoapps/content_tagging/tasks.py | 4 +- .../content_tagging/tests/test_api.py | 51 ++++++++----------- .../content_tagging/tests/test_tasks.py | 4 +- 9 files changed, 71 insertions(+), 97 deletions(-) create mode 100644 openedx/core/djangoapps/content_tagging/migrations/0008_remove_content_object_tag.py diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index ae0f62e3316e..ba767df78dc9 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -29,7 +29,7 @@ from cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers import load_services_for_studio from openedx.core.lib.xblock_utils import get_aside_from_xblock, is_xblock_aside from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration -from openedx.core.djangoapps.content_tagging.api import get_content_tags +from openedx.core.djangoapps.content_tagging.api import get_object_tags from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.exceptions import ItemNotFoundError # lint-amnesty, pylint: disable=wrong-import-order @@ -533,7 +533,7 @@ def get_unit_tags(usage_key): which already provides this grouping + sorting logic. """ # Get content tags from content tagging API - content_tags = get_content_tags(usage_key) + content_tags = get_object_tags(str(usage_key)) # Group content tags by taxonomy taxonomy_dict = {} diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 70aa7e2150da..6bc6397fe21a 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -5,11 +5,12 @@ import openedx_tagging.core.tagging.api as oel_tagging from django.db.models import Q, QuerySet, Exists, OuterRef -from openedx_tagging.core.tagging.models import Taxonomy +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey, UsageKey +from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy from organizations.models import Organization -from .models import ContentObjectTag, TaxonomyOrg -from .types import ContentKey +from .models import TaxonomyOrg def create_taxonomy( @@ -125,27 +126,11 @@ def get_unassigned_taxonomies(enabled=True) -> QuerySet: ) -def get_content_tags( - object_key: ContentKey, - taxonomy_id: int | None = None, -) -> QuerySet: - """ - Generates a list of content tags for a given object. - - Pass taxonomy to limit the returned object_tags to a specific taxonomy. - """ - return oel_tagging.get_object_tags( - object_id=str(object_key), - taxonomy_id=taxonomy_id, - object_tag_class=ContentObjectTag, - ) - - -def tag_content_object( - object_key: ContentKey, +def tag_object( + object_id: str, taxonomy: Taxonomy, - tags: list, -) -> QuerySet: + tags: list[str], +) -> QuerySet[ObjectTag]: """ This is the main API to use when you want to add/update/delete tags from a content object (e.g. an XBlock or course). @@ -160,19 +145,32 @@ def tag_content_object( Raises ValueError if the proposed tags are invalid for this taxonomy. Preserves existing (valid) tags, adds new (valid) tags, and removes omitted (or invalid) tags. + + We require that this taxonomy is linked to the content object's "org" or linked to "all orgs" (None). """ + object_key: UsageKey | CourseKey + if not taxonomy.system_defined: # We require that this taxonomy is linked to the content object's "org" or linked to "all orgs" (None): + try: + object_key = UsageKey.from_string(object_id) + except InvalidKeyError: + try: + object_key = CourseKey.from_string(object_id) + except InvalidKeyError as e: + raise ValueError("object_id must be from a block or a course") from e + org_short_name = object_key.org # type: ignore if not taxonomy.taxonomyorg_set.filter(Q(org__short_name=org_short_name) | Q(org=None)).exists(): raise ValueError(f"The specified Taxonomy is not enabled for the content object's org ({org_short_name})") + oel_tagging.tag_object( + object_id=object_id, taxonomy=taxonomy, tags=tags, - object_id=str(object_key), - object_tag_class=ContentObjectTag, ) - return get_content_tags(object_key, taxonomy_id=taxonomy.id) + + return oel_tagging.get_object_tags(object_id, taxonomy_id=taxonomy.id) # Expose the oel_tagging APIs @@ -183,3 +181,4 @@ def tag_content_object( get_object_tag_counts = oel_tagging.get_object_tag_counts delete_object_tags = oel_tagging.delete_object_tags resync_object_tags = oel_tagging.resync_object_tags +get_object_tags = oel_tagging.get_object_tags diff --git a/openedx/core/djangoapps/content_tagging/migrations/0008_remove_content_object_tag.py b/openedx/core/djangoapps/content_tagging/migrations/0008_remove_content_object_tag.py new file mode 100644 index 000000000000..ecaeec7f8c8b --- /dev/null +++ b/openedx/core/djangoapps/content_tagging/migrations/0008_remove_content_object_tag.py @@ -0,0 +1,16 @@ +# Generated by Django 3.2.23 on 2024-01-30 21:15 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('content_tagging', '0007_system_defined_org_2'), + ] + + operations = [ + migrations.DeleteModel( + name='ContentObjectTag', + ), + ] diff --git a/openedx/core/djangoapps/content_tagging/models/__init__.py b/openedx/core/djangoapps/content_tagging/models/__init__.py index f330b8ae819b..4a25224b4bae 100644 --- a/openedx/core/djangoapps/content_tagging/models/__init__.py +++ b/openedx/core/djangoapps/content_tagging/models/__init__.py @@ -3,5 +3,4 @@ """ from .base import ( TaxonomyOrg, - ContentObjectTag, ) diff --git a/openedx/core/djangoapps/content_tagging/models/base.py b/openedx/core/djangoapps/content_tagging/models/base.py index 77061e46118a..3ce125d99af3 100644 --- a/openedx/core/djangoapps/content_tagging/models/base.py +++ b/openedx/core/djangoapps/content_tagging/models/base.py @@ -3,13 +3,10 @@ """ from __future__ import annotations -from django.core.exceptions import ValidationError from django.db import models from django.db.models import Q, QuerySet from django.utils.translation import gettext as _ -from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import LearningContextKey, UsageKey -from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy +from openedx_tagging.core.tagging.models import Taxonomy from organizations.models import Organization @@ -80,33 +77,3 @@ def get_organizations( if rels.filter(org=None).exists(): return list(Organization.objects.all()) return [rel.org for rel in rels] - - -class ContentObjectTag(ObjectTag): - """ - ObjectTag that requires an LearningContextKey or BlockUsageLocator as the object ID. - """ - - class Meta: - proxy = True - - @property - def object_key(self) -> UsageKey | LearningContextKey: - """ - Returns the object ID parsed as a UsageKey or LearningContextKey. - Raises InvalidKeyError object_id cannot be parse into one of those key types. - - Returns None if there's no object_id. - """ - try: - return LearningContextKey.from_string(self.object_id) - except InvalidKeyError: - return UsageKey.from_string(self.object_id) - - def clean(self): - super().clean() - # Make sure that object_id is a valid key - try: - self.object_key - except InvalidKeyError as err: - raise ValidationError("object_id is not a valid opaque key string.") from err 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..20bf5f8b3f07 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 @@ -1607,7 +1607,7 @@ def test_object_tags_query_count(self): """ object_key = self.courseA object_id = str(object_key) - tagging_api.tag_content_object(object_key=object_key, taxonomy=self.t1, tags=["anvil", "android"]) + 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}, diff --git a/openedx/core/djangoapps/content_tagging/tasks.py b/openedx/core/djangoapps/content_tagging/tasks.py index c566a020cbcb..d2201dd60dac 100644 --- a/openedx/core/djangoapps/content_tagging/tasks.py +++ b/openedx/core/djangoapps/content_tagging/tasks.py @@ -36,7 +36,7 @@ def _set_initial_language_tag(content_key: ContentKey, lang_code: str) -> None: """ lang_taxonomy = Taxonomy.objects.get(pk=LANGUAGE_TAXONOMY_ID).cast() - if lang_code and not api.get_content_tags(object_key=content_key, taxonomy_id=lang_taxonomy.id): + if lang_code and not api.get_object_tags(object_id=str(content_key), taxonomy_id=lang_taxonomy.id): try: lang_tag = lang_taxonomy.tag_for_external_id(lang_code) except api.oel_tagging.TagDoesNotExist: @@ -47,7 +47,7 @@ def _set_initial_language_tag(content_key: ContentKey, lang_code: str) -> None: default_lang_code, ) lang_tag = lang_taxonomy.tag_for_external_id(default_lang_code) - api.tag_content_object(content_key, lang_taxonomy, [lang_tag.value]) + api.tag_object(str(content_key), lang_taxonomy, [lang_tag.value]) def _delete_tags(content_object: ContentKey) -> None: diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index 9a297be968b1..5e2d0b9a10b0 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -1,7 +1,6 @@ """Tests for the Tagging models""" import ddt from django.test.testcases import TestCase -from opaque_keys.edx.keys import CourseKey, UsageKey from openedx_tagging.core.tagging.models import Tag from organizations.models import Organization @@ -59,39 +58,33 @@ def setUp(self): value="learning", ) # ObjectTags - self.all_orgs_course_tag = api.tag_content_object( - object_key=CourseKey.from_string("course-v1:OeX+DemoX+Demo_Course"), + self.all_orgs_course_tag = api.tag_object( + object_id="course-v1:OeX+DemoX+Demo_Course", taxonomy=self.taxonomy_all_orgs, tags=[self.tag_all_orgs.value], )[0] - self.all_orgs_block_tag = api.tag_content_object( - object_key=UsageKey.from_string( - "block-v1:Ax+DemoX+Demo_Course+type@vertical+block@abcde" - ), + self.all_orgs_block_tag = api.tag_object( + object_id="block-v1:Ax+DemoX+Demo_Course+type@vertical+block@abcde", taxonomy=self.taxonomy_all_orgs, tags=[self.tag_all_orgs.value], )[0] - self.both_orgs_course_tag = api.tag_content_object( - object_key=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), + self.both_orgs_course_tag = api.tag_object( + object_id="course-v1:Ax+DemoX+Demo_Course", taxonomy=self.taxonomy_both_orgs, tags=[self.tag_both_orgs.value], )[0] - self.both_orgs_block_tag = api.tag_content_object( - object_key=UsageKey.from_string( - "block-v1:OeX+DemoX+Demo_Course+type@video+block@abcde" - ), + self.both_orgs_block_tag = api.tag_object( + object_id="block-v1:OeX+DemoX+Demo_Course+type@video+block@abcde", taxonomy=self.taxonomy_both_orgs, tags=[self.tag_both_orgs.value], )[0] - self.one_org_block_tag = api.tag_content_object( - object_key=UsageKey.from_string( - "block-v1:OeX+DemoX+Demo_Course+type@html+block@abcde" - ), + self.one_org_block_tag = api.tag_object( + object_id="block-v1:OeX+DemoX+Demo_Course+type@html+block@abcde", taxonomy=self.taxonomy_one_org, tags=[self.tag_one_org.value], )[0] - self.disabled_course_tag = api.tag_content_object( - object_key=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), + self.disabled_course_tag = api.tag_object( + object_id="course-v1:Ax+DemoX+Demo_Course", taxonomy=self.taxonomy_disabled, tags=[self.tag_disabled.value], )[0] @@ -180,8 +173,8 @@ def test_get_content_tags_valid_for_org( object_tag = getattr(self, object_tag_attr) with self.assertNumQueries(1): valid_tags = list( - api.get_content_tags( - object_key=object_tag.object_key, + api.get_object_tags( + object_id=object_tag.object_id, taxonomy_id=taxonomy_id, ) ) @@ -205,8 +198,8 @@ def test_get_content_tags( object_tag = getattr(self, object_tag_attr) with self.assertNumQueries(1): valid_tags = list( - api.get_content_tags( - object_key=object_tag.object_key, + api.get_object_tags( + object_id=object_tag.object_id, taxonomy_id=taxonomy_id, ) ) @@ -230,21 +223,21 @@ def test_cannot_tag_across_orgs(self): taxonomy = self.taxonomy_one_org tags = [self.tag_one_org.value] with self.assertRaises(ValueError) as exc: - api.tag_content_object( - object_key=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), + api.tag_object( + object_id="course-v1:Ax+DemoX+Demo_Course", taxonomy=taxonomy, tags=tags, ) assert "The specified Taxonomy is not enabled for the content object's org (Ax)" in str(exc.exception) # But this will work fine: - api.tag_content_object( - object_key=CourseKey.from_string("course-v1:OeX+DemoX+Demo_Course"), + api.tag_object( + object_id="course-v1:OeX+DemoX+Demo_Course", taxonomy=taxonomy, tags=tags, ) # As will this: - api.tag_content_object( - object_key=CourseKey.from_string("course-v1:Ax+DemoX+Demo_Course"), + api.tag_object( + object_id="course-v1:Ax+DemoX+Demo_Course", taxonomy=self.taxonomy_both_orgs, tags=[self.tag_both_orgs.value], ) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_tasks.py b/openedx/core/djangoapps/content_tagging/tests/test_tasks.py index 7c8527e3b5e8..e168567c303d 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_tasks.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_tasks.py @@ -73,7 +73,7 @@ def _check_tag(self, object_key: ContentKey, taxonomy_id: int, value: str | None If value is None, check if the ObjectTag does not exists """ - object_tags = list(api.get_content_tags(object_key, taxonomy_id=taxonomy_id)) + object_tags = list(api.get_object_tags(str(object_key), taxonomy_id=taxonomy_id)) object_tag = object_tags[0] if len(object_tags) == 1 else None if len(object_tags) > 1: raise ValueError("Found too many object tags") @@ -166,7 +166,7 @@ def test_update_course(self): # Simulates user manually changing a tag lang_taxonomy = Taxonomy.objects.get(pk=LANGUAGE_TAXONOMY_ID) - api.tag_content_object(course.id, lang_taxonomy, ["Español (España)"]) + api.tag_object(str(course.id), lang_taxonomy, ["Español (España)"]) # Update course language course.language = "en" From 739fdb5edbc72f0845f6087b17b0c93c9cf70b99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 31 Jan 2024 17:04:40 -0300 Subject: [PATCH 02/12] refactor: remove tag_object function and add cross-org check to rules --- .../core/djangoapps/content_tagging/api.py | 54 +---------------- .../rest_api/v1/tests/test_views.py | 14 ++++- .../core/djangoapps/content_tagging/rules.py | 46 +++++++++++++-- .../core/djangoapps/content_tagging/tasks.py | 6 +- .../content_tagging/tests/test_api.py | 58 ++++++++----------- .../content_tagging/tests/test_rules.py | 4 +- .../content_tagging/tests/test_tasks.py | 6 +- 7 files changed, 94 insertions(+), 94 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/api.py b/openedx/core/djangoapps/content_tagging/api.py index 6bc6397fe21a..5f3a8bf3175c 100644 --- a/openedx/core/djangoapps/content_tagging/api.py +++ b/openedx/core/djangoapps/content_tagging/api.py @@ -4,10 +4,8 @@ from __future__ import annotations import openedx_tagging.core.tagging.api as oel_tagging -from django.db.models import Q, QuerySet, Exists, OuterRef -from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import CourseKey, UsageKey -from openedx_tagging.core.tagging.models import ObjectTag, Taxonomy +from django.db.models import QuerySet, Exists, OuterRef +from openedx_tagging.core.tagging.models import Taxonomy from organizations.models import Organization from .models import TaxonomyOrg @@ -126,53 +124,6 @@ def get_unassigned_taxonomies(enabled=True) -> QuerySet: ) -def tag_object( - object_id: str, - taxonomy: Taxonomy, - tags: list[str], -) -> QuerySet[ObjectTag]: - """ - This is the main API to use when you want to add/update/delete tags from a content object (e.g. an XBlock or - course). - - It works one "Taxonomy" at a time, i.e. one field at a time, so you can set call it with taxonomy=Keywords, - tags=["gravity", "newton"] to replace any "Keywords" [Taxonomy] tags on the given content object with "gravity" and - "newton". Doing so to change the "Keywords" Taxonomy won't affect other Taxonomy's tags (other fields) on the - object, such as "Language: [en]" or "Difficulty: [hard]". - - If it's a free-text taxonomy, then the list should be a list of tag values. - Otherwise, it should be a list of existing Tag IDs. - - Raises ValueError if the proposed tags are invalid for this taxonomy. - Preserves existing (valid) tags, adds new (valid) tags, and removes omitted (or invalid) tags. - - We require that this taxonomy is linked to the content object's "org" or linked to "all orgs" (None). - """ - object_key: UsageKey | CourseKey - - if not taxonomy.system_defined: - # We require that this taxonomy is linked to the content object's "org" or linked to "all orgs" (None): - try: - object_key = UsageKey.from_string(object_id) - except InvalidKeyError: - try: - object_key = CourseKey.from_string(object_id) - except InvalidKeyError as e: - raise ValueError("object_id must be from a block or a course") from e - - org_short_name = object_key.org # type: ignore - if not taxonomy.taxonomyorg_set.filter(Q(org__short_name=org_short_name) | Q(org=None)).exists(): - raise ValueError(f"The specified Taxonomy is not enabled for the content object's org ({org_short_name})") - - oel_tagging.tag_object( - object_id=object_id, - taxonomy=taxonomy, - tags=tags, - ) - - return oel_tagging.get_object_tags(object_id, taxonomy_id=taxonomy.id) - - # Expose the oel_tagging APIs get_taxonomy = oel_tagging.get_taxonomy @@ -182,3 +133,4 @@ def tag_object( delete_object_tags = oel_tagging.delete_object_tags resync_object_tags = oel_tagging.resync_object_tags get_object_tags = oel_tagging.get_object_tags +tag_object = oel_tagging.tag_object 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 20bf5f8b3f07..2626e407b2cc 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 @@ -1534,6 +1534,18 @@ def test_tag_xblock_invalid(self, user_attr, taxonomy_attr): response = self.client.put(url, {"tags": ["invalid"]}, format="json") assert response.status_code == status.HTTP_400_BAD_REQUEST + def test_tag_cross_org(self): + """ + Tests that we cannot add a taxonomy from orgA to an object from orgB + """ + self.client.force_authenticate(user=self.staff) + + url = OBJECT_TAG_UPDATE_URL.format(object_id=self.courseB, taxonomy_id=self.tA1.pk) + + response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") + + assert response.status_code == status.HTTP_403_FORBIDDEN + @ddt.data( "courseB", "xblockB", @@ -1615,7 +1627,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(10): # TODO Why so many queries? 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 af6bdbeb9435..3dfa3ae09359 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -6,10 +6,11 @@ import django.contrib.auth.models import openedx_tagging.core.tagging.rules as oel_tagging -from organizations.models import Organization import rules +from django.db.models import Q from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey +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 @@ -262,6 +263,42 @@ def can_view_object_tag_objectid(user: UserType, object_id: str) -> bool: return has_studio_read_access(user, course_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 == "": + return True + + # Library locators are also subclasses of CourseKey + context_key: CourseKey + try: + context_key = CourseKey.from_string(perm_obj.object_id) + except InvalidKeyError as course_key_error: + try: + usage_key = UsageKey.from_string(perm_obj.object_id) + if not isinstance(usage_key.context_key, CourseKey): + raise ValueError("object_id must be child of a course or a library") from course_key_error + context_key = usage_key.context_key + except InvalidKeyError as usage_key_error: + raise ValueError("object_id must be from a LearningContextKey or a UsageKey") from usage_key_error + + org_short_name = context_key.org + return perm_obj.taxonomy.taxonomyorg_set.filter(Q(org__short_name=org_short_name) | Q(org=None)).exists() + + @rules.predicate def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None) -> bool: """ @@ -292,10 +329,11 @@ 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_object_tag", oel_tagging.can_change_object_tag) -rules.set_perm("oel_tagging.change_objecttag", oel_tagging.can_change_object_tag) -rules.set_perm("oel_tagging.delete_objecttag", 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.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 # to include Organization and objects permissions. diff --git a/openedx/core/djangoapps/content_tagging/tasks.py b/openedx/core/djangoapps/content_tagging/tasks.py index d2201dd60dac..1f07abc1bcbc 100644 --- a/openedx/core/djangoapps/content_tagging/tasks.py +++ b/openedx/core/djangoapps/content_tagging/tasks.py @@ -47,7 +47,11 @@ def _set_initial_language_tag(content_key: ContentKey, lang_code: str) -> None: default_lang_code, ) lang_tag = lang_taxonomy.tag_for_external_id(default_lang_code) - api.tag_object(str(content_key), lang_taxonomy, [lang_tag.value]) + api.tag_object( + object_id=str(content_key), + taxonomy=lang_taxonomy, + tags=[lang_tag.value], + ) def _delete_tags(content_object: ContentKey) -> None: diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index 5e2d0b9a10b0..807b7d8e1dc6 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -58,35 +58,53 @@ def setUp(self): value="learning", ) # ObjectTags - self.all_orgs_course_tag = api.tag_object( + api.tag_object( object_id="course-v1:OeX+DemoX+Demo_Course", taxonomy=self.taxonomy_all_orgs, tags=[self.tag_all_orgs.value], + ) + self.all_orgs_course_tag = api.get_object_tags( + object_id="course-v1:OeX+DemoX+Demo_Course", )[0] - self.all_orgs_block_tag = api.tag_object( + api.tag_object( object_id="block-v1:Ax+DemoX+Demo_Course+type@vertical+block@abcde", taxonomy=self.taxonomy_all_orgs, tags=[self.tag_all_orgs.value], + ) + self.all_orgs_block_tag = api.get_object_tags( + object_id="block-v1:Ax+DemoX+Demo_Course+type@vertical+block@abcde", )[0] - self.both_orgs_course_tag = api.tag_object( + api.tag_object( object_id="course-v1:Ax+DemoX+Demo_Course", taxonomy=self.taxonomy_both_orgs, tags=[self.tag_both_orgs.value], + ) + self.both_orgs_course_tag = api.get_object_tags( + object_id="course-v1:Ax+DemoX+Demo_Course", )[0] - self.both_orgs_block_tag = api.tag_object( + api.tag_object( object_id="block-v1:OeX+DemoX+Demo_Course+type@video+block@abcde", taxonomy=self.taxonomy_both_orgs, tags=[self.tag_both_orgs.value], + ) + self.both_orgs_block_tag = api.get_object_tags( + object_id="block-v1:OeX+DemoX+Demo_Course+type@video+block@abcde", )[0] - self.one_org_block_tag = api.tag_object( + api.tag_object( object_id="block-v1:OeX+DemoX+Demo_Course+type@html+block@abcde", taxonomy=self.taxonomy_one_org, tags=[self.tag_one_org.value], + ) + self.one_org_block_tag = api.get_object_tags( + object_id="block-v1:OeX+DemoX+Demo_Course+type@html+block@abcde", )[0] - self.disabled_course_tag = api.tag_object( + api.tag_object( object_id="course-v1:Ax+DemoX+Demo_Course", taxonomy=self.taxonomy_disabled, tags=[self.tag_disabled.value], + ) + self.disabled_course_tag = api.get_object_tags( + object_id="course-v1:Ax+DemoX+Demo_Course", )[0] self.taxonomy_disabled.enabled = False self.taxonomy_disabled.save() @@ -213,31 +231,3 @@ def test_get_tags(self): assert result[0]["_id"] == self.tag_all_orgs.id assert result[0]["parent_value"] is None assert result[0]["depth"] == 0 - - def test_cannot_tag_across_orgs(self): - """ - Ensure that I cannot apply tags from a taxonomy that's linked to another - org. - """ - # This taxonomy is only linked to the "OpenedX org", so it can't be used for "Axim" content. - taxonomy = self.taxonomy_one_org - tags = [self.tag_one_org.value] - with self.assertRaises(ValueError) as exc: - api.tag_object( - object_id="course-v1:Ax+DemoX+Demo_Course", - taxonomy=taxonomy, - tags=tags, - ) - assert "The specified Taxonomy is not enabled for the content object's org (Ax)" in str(exc.exception) - # But this will work fine: - api.tag_object( - object_id="course-v1:OeX+DemoX+Demo_Course", - taxonomy=taxonomy, - tags=tags, - ) - # As will this: - api.tag_object( - object_id="course-v1:Ax+DemoX+Demo_Course", - taxonomy=self.taxonomy_both_orgs, - tags=[self.tag_both_orgs.value], - ) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_rules.py b/openedx/core/djangoapps/content_tagging/tests/test_rules.py index d0aba1f91fb3..f32c14c28bb7 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_rules.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_rules.py @@ -534,10 +534,10 @@ def test_object_tag_disabled_taxonomy(self, perm, tag_attr): ) @ddt.unpack def test_object_tag_no_orgs(self, perm, tag_attr): - """Only staff & superusers can create/edit an ObjectTag with a no-org Taxonomy""" + """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) diff --git a/openedx/core/djangoapps/content_tagging/tests/test_tasks.py b/openedx/core/djangoapps/content_tagging/tests/test_tasks.py index e168567c303d..52a6fcb5b883 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_tasks.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_tasks.py @@ -166,7 +166,11 @@ def test_update_course(self): # Simulates user manually changing a tag lang_taxonomy = Taxonomy.objects.get(pk=LANGUAGE_TAXONOMY_ID) - api.tag_object(str(course.id), lang_taxonomy, ["Español (España)"]) + api.tag_object( + object_id=str(course.id), + taxonomy=lang_taxonomy, + tags=["Español (España)"] + ) # Update course language course.language = "en" From 1a4c85829b51d8a7a5f2231c562058e863066b68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 1 Feb 2024 14:56:16 -0300 Subject: [PATCH 03/12] fix: allow taxonomy admins use no-org taxonomies --- .../rest_api/v1/tests/test_views.py | 44 ++++++++++++++++--- .../core/djangoapps/content_tagging/rules.py | 4 ++ .../content_tagging/tests/test_rules.py | 2 +- 3 files changed, 43 insertions(+), 7 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 2626e407b2cc..49c4440d18fd 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,10 +125,16 @@ def _setUp_users(self): self.staffA = User.objects.create( username="staffA", - email="userA@example.com", + email="staffA@example.com", ) update_org_role(self.staff, OrgStaffRole, self.staffA, [self.orgA.short_name]) + self.staffB = User.objects.create( + username="staffB", + email="staffB@example.com", + ) + update_org_role(self.staff, OrgStaffRole, self.staffB, [self.orgB.short_name]) + self.content_creatorA = User.objects.create( username="content_creatorA", email="content_creatorA@example.com", @@ -1534,17 +1540,43 @@ def test_tag_xblock_invalid(self, user_attr, taxonomy_attr): response = self.client.put(url, {"tags": ["invalid"]}, format="json") assert response.status_code == status.HTTP_400_BAD_REQUEST - def test_tag_cross_org(self): + @ddt.data( + ("staff", status.HTTP_200_OK), + ("staffA", status.HTTP_403_FORBIDDEN), + ("staffB", status.HTTP_403_FORBIDDEN), + ) + @ddt.unpack + def test_tag_cross_org(self, user_attr, expected_status): """ - Tests that we cannot add a taxonomy from orgA to an object from orgB + Tests that only global admins can add a taxonomy from orgA to an object from orgB """ - self.client.force_authenticate(user=self.staff) + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) url = OBJECT_TAG_UPDATE_URL.format(object_id=self.courseB, taxonomy_id=self.tA1.pk) response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") - assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.status_code == expected_status + + @ddt.data( + ("staff", status.HTTP_200_OK), + ("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 + """ + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + + url = OBJECT_TAG_UPDATE_URL.format(object_id=self.courseA, taxonomy_id=self.ot1.pk) + + response = self.client.put(url, {"tags": []}, format="json") + + assert response.status_code == expected_status @ddt.data( "courseB", @@ -1627,7 +1659,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(10): # TODO Why so many queries? + with self.assertNumQueries(7): # TODO Why so many queries? 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 3dfa3ae09359..d9bb5fdf9412 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -282,6 +282,10 @@ def can_change_object_tag( if perm_obj.object_id == "": return True + # Taxonomy admins can tag any object using any taxonomy + if oel_tagging.is_taxonomy_admin(user): + return True + # Library locators are also subclasses of CourseKey context_key: CourseKey try: diff --git a/openedx/core/djangoapps/content_tagging/tests/test_rules.py b/openedx/core/djangoapps/content_tagging/tests/test_rules.py index f32c14c28bb7..8dd8db125843 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 not self.staff.has_perm(perm, object_tag) + assert 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 51be5689e04ce070d654cbfed3836dae9fc0fd45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 1 Feb 2024 15:10:49 -0300 Subject: [PATCH 04/12] fix: skip check if taxonomy not provided --- openedx/core/djangoapps/content_tagging/rules.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index d9bb5fdf9412..74c11b2c5f02 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -282,6 +282,10 @@ def can_change_object_tag( if perm_obj.object_id == "": 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 From dfb908ea59b95002123a7a5942a5bfe76db14e0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Fri, 2 Feb 2024 14:34:27 -0300 Subject: [PATCH 05/12] refactor: add utils module to handle opaque keys --- .../core/djangoapps/content_tagging/rules.py | 38 ++++--------------- .../core/djangoapps/content_tagging/tasks.py | 6 +-- .../core/djangoapps/content_tagging/types.py | 4 +- .../core/djangoapps/content_tagging/utils.py | 37 ++++++++++++++++++ 4 files changed, 50 insertions(+), 35 deletions(-) create mode 100644 openedx/core/djangoapps/content_tagging/utils.py diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 74c11b2c5f02..10f5eaca1d16 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -8,8 +8,6 @@ import openedx_tagging.core.tagging.rules as oel_tagging import rules from django.db.models import Q -from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import CourseKey, UsageKey from organizations.models import Organization from common.djangoapps.student.auth import has_studio_read_access, has_studio_write_access @@ -25,6 +23,7 @@ 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 UserType = Union[django.contrib.auth.models.User, django.contrib.auth.models.AnonymousUser] @@ -221,15 +220,10 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: """ if not object_id: return True - try: - usage_key = UsageKey.from_string(object_id) - if not usage_key.course_key.is_course: - raise ValueError("object_id must be from a block or a course") - course_key = usage_key.course_key - except InvalidKeyError: - course_key = CourseKey.from_string(object_id) - return has_studio_write_access(user, course_key) + context_key = get_context_key_from_key_string(object_id) + + return has_studio_write_access(user, context_key) @rules.predicate @@ -252,15 +246,10 @@ def can_view_object_tag_objectid(user: UserType, object_id: str) -> bool: """ if not object_id: raise ValueError("object_id must be provided") - try: - usage_key = UsageKey.from_string(object_id) - if not usage_key.course_key.is_course: - raise ValueError("object_id must be from a block or a course") - course_key = usage_key.course_key - except InvalidKeyError: - course_key = CourseKey.from_string(object_id) - return has_studio_read_access(user, course_key) + context_key = get_context_key_from_key_string(object_id) + + return has_studio_read_access(user, context_key) @rules.predicate @@ -290,18 +279,7 @@ def can_change_object_tag( if oel_tagging.is_taxonomy_admin(user): return True - # Library locators are also subclasses of CourseKey - context_key: CourseKey - try: - context_key = CourseKey.from_string(perm_obj.object_id) - except InvalidKeyError as course_key_error: - try: - usage_key = UsageKey.from_string(perm_obj.object_id) - if not isinstance(usage_key.context_key, CourseKey): - raise ValueError("object_id must be child of a course or a library") from course_key_error - context_key = usage_key.context_key - except InvalidKeyError as usage_key_error: - raise ValueError("object_id must be from a LearningContextKey or a UsageKey") from usage_key_error + 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() diff --git a/openedx/core/djangoapps/content_tagging/tasks.py b/openedx/core/djangoapps/content_tagging/tasks.py index 1f07abc1bcbc..bd90aad6eaba 100644 --- a/openedx/core/djangoapps/content_tagging/tasks.py +++ b/openedx/core/djangoapps/content_tagging/tasks.py @@ -10,7 +10,7 @@ from django.conf import settings from django.contrib.auth import get_user_model from edx_django_utils.monitoring import set_code_owner_attribute -from opaque_keys.edx.keys import LearningContextKey, UsageKey +from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys.edx.locator import LibraryUsageLocatorV2 from openedx_tagging.core.tagging.models import Taxonomy @@ -69,7 +69,7 @@ def update_course_tags(course_key_str: str) -> bool: course_key_str (str): identifier of the Course """ try: - course_key = LearningContextKey.from_string(course_key_str) + course_key = CourseKey.from_string(course_key_str) log.info("Updating tags for Course with id: %s", course_key) @@ -94,7 +94,7 @@ def delete_course_tags(course_key_str: str) -> bool: course_key_str (str): identifier of the Course """ try: - course_key = LearningContextKey.from_string(course_key_str) + course_key = CourseKey.from_string(course_key_str) log.info("Deleting tags for Course with id: %s", course_key) diff --git a/openedx/core/djangoapps/content_tagging/types.py b/openedx/core/djangoapps/content_tagging/types.py index 3a9f6ec54935..2a065f13f5d6 100644 --- a/openedx/core/djangoapps/content_tagging/types.py +++ b/openedx/core/djangoapps/content_tagging/types.py @@ -3,6 +3,6 @@ """ from typing import Union -from opaque_keys.edx.keys import LearningContextKey, UsageKey +from opaque_keys.edx.keys import CourseKey, UsageKey -ContentKey = Union[LearningContextKey, UsageKey] +ContentKey = Union[CourseKey, UsageKey] diff --git a/openedx/core/djangoapps/content_tagging/utils.py b/openedx/core/djangoapps/content_tagging/utils.py new file mode 100644 index 000000000000..140c58c94259 --- /dev/null +++ b/openedx/core/djangoapps/content_tagging/utils.py @@ -0,0 +1,37 @@ +""" +Utils functions for tagging +""" +from __future__ import annotations + +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey, UsageKey + +from .types import ContentKey + + +def get_content_key_from_string(key_str: str) -> ContentKey: + """ + Get content key from string + """ + # Library locators are also subclasses of CourseKey + try: + return CourseKey.from_string(key_str) + except InvalidKeyError: + try: + return UsageKey.from_string(key_str) + except InvalidKeyError as usage_key_error: + raise ValueError("object_id must be from a CourseKey or a UsageKey") from usage_key_error + + +def get_context_key_from_key_string(key_str: str) -> CourseKey: + """ + Get context key from an key string + """ + content_key = get_content_key_from_string(key_str) + if isinstance(content_key, CourseKey): + return content_key + + if not isinstance(content_key.context_key, CourseKey): + raise ValueError("context must be from a CourseKey") + + return content_key.context_key From 0278ad87ad6709ff25ad845e81878139f9a1fbb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 6 Feb 2024 17:58:09 -0300 Subject: [PATCH 06/12] test: add tests for tagging LibraryV2 --- .../rest_api/v1/tests/test_views.py | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) 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 49c4440d18fd..798246d72924 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 @@ -108,6 +108,7 @@ def _setUp_library(self): title="Library Org A", description="This is a library from Org A", ) + self.libraryA = str(self.content_libraryA.key) def _setUp_users(self): """ @@ -1540,6 +1541,91 @@ def test_tag_xblock_invalid(self, user_attr, taxonomy_attr): response = self.client.put(url, {"tags": ["invalid"]}, format="json") assert response.status_code == status.HTTP_400_BAD_REQUEST + @ddt.data( + # userA and userS are staff in libraryA and can tag using enabled taxonomies + ("user", "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("staffA", "tA1", ["Tag 1"], status.HTTP_200_OK), + ("staff", "tA1", ["Tag 1"], status.HTTP_200_OK), + ("user", "tA1", [], status.HTTP_403_FORBIDDEN), + ("staffA", "tA1", [], status.HTTP_200_OK), + ("staff", "tA1", [], status.HTTP_200_OK), + ("user", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), + ("staffA", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + ("staff", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + ("user", "open_taxonomy", ["tag1"], status.HTTP_403_FORBIDDEN), + ("staffA", "open_taxonomy", ["tag1"], status.HTTP_200_OK), + ("staff", "open_taxonomy", ["tag1"], status.HTTP_200_OK), + ) + @ddt.unpack + def test_tag_library(self, user_attr, taxonomy_attr, tag_values, expected_status): + """ + Tests that only staff and org level users can tag libraries + """ + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + + taxonomy = getattr(self, taxonomy_attr) + + url = OBJECT_TAG_UPDATE_URL.format(object_id=self.libraryA, taxonomy_id=taxonomy.pk) + + response = self.client.put(url, {"tags": tag_values}, format="json") + + assert response.status_code == expected_status + if status.is_success(expected_status): + tags_by_taxonomy = response.data[str(self.libraryA)]["taxonomies"] + if tag_values: + response_taxonomy = tags_by_taxonomy[0] + assert response_taxonomy["name"] == taxonomy.name + response_tags = response_taxonomy["tags"] + assert [t["value"] for t in response_tags] == tag_values + else: + assert tags_by_taxonomy == [] # No tags are set from any taxonomy + + # Check that re-fetching the tags returns what we set + new_response = self.client.get(url, format="json") + assert status.is_success(new_response.status_code) + assert new_response.data == response.data + + @ddt.data( + "staffA", + "staff", + ) + def test_tag_library_disabled_taxonomy(self, user_attr): + """ + Nobody can use disable taxonomies to tag objects + """ + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + + disabled_taxonomy = self.tA2 + assert disabled_taxonomy.enabled is False + + url = OBJECT_TAG_UPDATE_URL.format(object_id=self.libraryA, taxonomy_id=disabled_taxonomy.pk) + response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") + + assert response.status_code == status.HTTP_403_FORBIDDEN + + @ddt.data( + ("staffA", "tA1"), + ("staff", "tA1"), + ("staffA", "multiple_taxonomy"), + ("staff", "multiple_taxonomy"), + ) + @ddt.unpack + def test_tag_library_invalid(self, user_attr, taxonomy_attr): + """ + Tests that nobody can add invalid tags to a library using a closed taxonomy + """ + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + + taxonomy = getattr(self, taxonomy_attr) + + url = OBJECT_TAG_UPDATE_URL.format(object_id=self.libraryA, taxonomy_id=taxonomy.pk) + + response = self.client.put(url, {"tags": ["invalid"]}, format="json") + assert response.status_code == status.HTTP_400_BAD_REQUEST + @ddt.data( ("staff", status.HTTP_200_OK), ("staffA", status.HTTP_403_FORBIDDEN), @@ -1612,6 +1698,9 @@ def test_tag_unauthorized(self, objectid_attr): assert response.status_code == status.HTTP_401_UNAUTHORIZED def test_get_tags(self): + """ + Test that we can get tags for an object + """ self.client.force_authenticate(user=self.staffA) taxonomy = self.multiple_taxonomy tag_values = ["Tag 1", "Tag 2"] From 38e03ce221eccbb0774ec500c55ccb4e775132cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 6 Feb 2024 17:58:41 -0300 Subject: [PATCH 07/12] fix: types --- .../core/djangoapps/content_tagging/types.py | 3 ++- .../core/djangoapps/content_tagging/utils.py | 24 ++++++++++++------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/types.py b/openedx/core/djangoapps/content_tagging/types.py index 2a065f13f5d6..44b3fd3e8a59 100644 --- a/openedx/core/djangoapps/content_tagging/types.py +++ b/openedx/core/djangoapps/content_tagging/types.py @@ -4,5 +4,6 @@ from typing import Union from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.locator import LibraryLocatorV2 -ContentKey = Union[CourseKey, UsageKey] +ContentKey = Union[LibraryLocatorV2, CourseKey, UsageKey] diff --git a/openedx/core/djangoapps/content_tagging/utils.py b/openedx/core/djangoapps/content_tagging/utils.py index 140c58c94259..808f1d29fc81 100644 --- a/openedx/core/djangoapps/content_tagging/utils.py +++ b/openedx/core/djangoapps/content_tagging/utils.py @@ -5,6 +5,7 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey +from opaque_keys.edx.locator import LibraryLocatorV2 from .types import ContentKey @@ -13,25 +14,30 @@ def get_content_key_from_string(key_str: str) -> ContentKey: """ Get content key from string """ - # Library locators are also subclasses of CourseKey try: return CourseKey.from_string(key_str) except InvalidKeyError: try: - return UsageKey.from_string(key_str) - except InvalidKeyError as usage_key_error: - raise ValueError("object_id must be from a CourseKey or a UsageKey") from usage_key_error + return LibraryLocatorV2.from_string(key_str) + except InvalidKeyError: + try: + return UsageKey.from_string(key_str) + except InvalidKeyError as usage_key_error: + raise ValueError("object_id must be a CourseKey, LibraryLocatorV2 or a UsageKey") from usage_key_error -def get_context_key_from_key_string(key_str: str) -> CourseKey: +def get_context_key_from_key_string(key_str: str) -> CourseKey | LibraryLocatorV2: """ Get context key from an key string """ content_key = get_content_key_from_string(key_str) - if isinstance(content_key, CourseKey): + if isinstance(content_key, CourseKey) or isinstance(content_key, LibraryLocatorV2): return content_key - if not isinstance(content_key.context_key, CourseKey): - raise ValueError("context must be from a CourseKey") + context_key = content_key.context_key + + if isinstance(context_key, CourseKey) or isinstance(context_key, LibraryLocatorV2): + return context_key + + raise ValueError("context must be a CourseKey or a LibraryLocatorV2") - return content_key.context_key From 9975a78d7c2ba4faebe5c9889e8da434ce26509d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 6 Feb 2024 18:06:28 -0300 Subject: [PATCH 08/12] docs: add comments --- openedx/core/djangoapps/content_tagging/utils.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/openedx/core/djangoapps/content_tagging/utils.py b/openedx/core/djangoapps/content_tagging/utils.py index 808f1d29fc81..1630ea268456 100644 --- a/openedx/core/djangoapps/content_tagging/utils.py +++ b/openedx/core/djangoapps/content_tagging/utils.py @@ -31,9 +31,11 @@ def get_context_key_from_key_string(key_str: str) -> CourseKey | LibraryLocatorV Get context key from an key string """ content_key = get_content_key_from_string(key_str) + # If the content key is a CourseKey or a LibraryLocatorV2, return it if isinstance(content_key, CourseKey) or isinstance(content_key, LibraryLocatorV2): return content_key + # If the content key is a UsageKey, return the context key context_key = content_key.context_key if isinstance(context_key, CourseKey) or isinstance(context_key, LibraryLocatorV2): From cc6e2fd8fc6d24f28d710c16a794f17ee3725107 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 6 Feb 2024 18:48:12 -0300 Subject: [PATCH 09/12] fix: pylint --- openedx/core/djangoapps/content_tagging/utils.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/openedx/core/djangoapps/content_tagging/utils.py b/openedx/core/djangoapps/content_tagging/utils.py index 1630ea268456..7e8efa9a8933 100644 --- a/openedx/core/djangoapps/content_tagging/utils.py +++ b/openedx/core/djangoapps/content_tagging/utils.py @@ -32,14 +32,13 @@ def get_context_key_from_key_string(key_str: str) -> CourseKey | LibraryLocatorV """ content_key = get_content_key_from_string(key_str) # If the content key is a CourseKey or a LibraryLocatorV2, return it - if isinstance(content_key, CourseKey) or isinstance(content_key, LibraryLocatorV2): + if isinstance(content_key, (CourseKey, LibraryLocatorV2)): return content_key # If the content key is a UsageKey, return the context key context_key = content_key.context_key - if isinstance(context_key, CourseKey) or isinstance(context_key, LibraryLocatorV2): + if isinstance(context_key, (CourseKey, LibraryLocatorV2)): return context_key raise ValueError("context must be a CourseKey or a LibraryLocatorV2") - From 2791963685d7aa165d90df926c5b27e9e571d022 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 8 Feb 2024 14:28:50 -0300 Subject: [PATCH 10/12] fix: add test to invalid object id tagging --- .../content_tagging/rest_api/v1/tests/test_views.py | 11 +++++++++++ openedx/core/djangoapps/content_tagging/rules.py | 10 ++++++++-- 2 files changed, 19 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 798246d72924..dbde95eb1fb5 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 @@ -1697,6 +1697,17 @@ def test_tag_unauthorized(self, objectid_attr): assert response.status_code == status.HTTP_401_UNAUTHORIZED + def test_tag_invalid_object(self): + """ + Test that we cannot tag an object that is not a CouseKey, LibraryLocatorV2 or UsageKey + """ + url = OBJECT_TAG_UPDATE_URL.format(object_id='invalid_key', taxonomy_id=self.tA1.pk) + self.client.force_authenticate(user=self.staff) + + response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") + + assert response.status_code == status.HTTP_403_FORBIDDEN + def test_get_tags(self): """ Test that we can get tags for an object diff --git a/openedx/core/djangoapps/content_tagging/rules.py b/openedx/core/djangoapps/content_tagging/rules.py index 10f5eaca1d16..fef71eeaf5de 100644 --- a/openedx/core/djangoapps/content_tagging/rules.py +++ b/openedx/core/djangoapps/content_tagging/rules.py @@ -221,7 +221,10 @@ def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: if not object_id: return True - context_key = get_context_key_from_key_string(object_id) + try: + context_key = get_context_key_from_key_string(object_id) + except ValueError: + return False return has_studio_write_access(user, context_key) @@ -247,7 +250,10 @@ def can_view_object_tag_objectid(user: UserType, object_id: str) -> bool: if not object_id: raise ValueError("object_id must be provided") - context_key = get_context_key_from_key_string(object_id) + try: + context_key = get_context_key_from_key_string(object_id) + except ValueError: + return False return has_studio_read_access(user, context_key) From 73970f38111ced09377393dd878261924606bad0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 8 Feb 2024 14:38:28 -0300 Subject: [PATCH 11/12] docs: fix typo Co-authored-by: Braden MacDonald --- .../djangoapps/content_tagging/rest_api/v1/tests/test_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 dbde95eb1fb5..b476eeb6d873 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 @@ -1592,7 +1592,7 @@ def test_tag_library(self, user_attr, taxonomy_attr, tag_values, expected_status ) def test_tag_library_disabled_taxonomy(self, user_attr): """ - Nobody can use disable taxonomies to tag objects + Nobody can use disabled taxonomies to tag objects """ user = getattr(self, user_attr) self.client.force_authenticate(user=user) From 78b3d53b7ca1909ae83ed0472be40696de3e05b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 8 Feb 2024 14:43:07 -0300 Subject: [PATCH 12/12] docs: fix test comments --- .../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 b476eeb6d873..29c401b4f376 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 @@ -1373,7 +1373,7 @@ class TestObjectTagViewSet(TestObjectTagMixin, APITestCase): """ @ddt.data( - # userA and userS are staff in courseA and can tag using enabled taxonomies + # staffA and staff are staff in courseA and can tag using enabled taxonomies ("user", "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), ("staffA", "tA1", ["Tag 1"], status.HTTP_200_OK), ("staff", "tA1", ["Tag 1"], status.HTTP_200_OK), @@ -1458,7 +1458,7 @@ def test_tag_course_invalid(self, user_attr, taxonomy_attr): assert response.status_code == status.HTTP_400_BAD_REQUEST @ddt.data( - # userA and userS are staff in courseA (owner of xblockA) and can tag using any taxonomies + # staffA and staff are staff in courseA (owner of xblockA) and can tag using any taxonomies ("user", "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), ("staffA", "tA1", ["Tag 1"], status.HTTP_200_OK), ("staff", "tA1", ["Tag 1"], status.HTTP_200_OK), @@ -1542,7 +1542,7 @@ def test_tag_xblock_invalid(self, user_attr, taxonomy_attr): assert response.status_code == status.HTTP_400_BAD_REQUEST @ddt.data( - # userA and userS are staff in libraryA and can tag using enabled taxonomies + # staffA and staff are staff in libraryA and can tag using enabled taxonomies ("user", "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), ("staffA", "tA1", ["Tag 1"], status.HTTP_200_OK), ("staff", "tA1", ["Tag 1"], status.HTTP_200_OK),