From 3c3306ce855d508ccae2d5b38607e47afe90db27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 18 Sep 2023 15:07:00 -0300 Subject: [PATCH] feat: implement tag object rest api in cms (#33181) --- .../content_tagging/rest_api/v1/filters.py | 4 +- .../rest_api/v1/tests/test_views.py | 220 ++++++++++++++- openedx/features/content_tagging/rules.py | 110 +++----- .../content_tagging/tests/test_rules.py | 265 ++++++++++++------ requirements/constraints.txt | 3 + requirements/edx/base.txt | 6 +- requirements/edx/development.txt | 3 +- requirements/edx/doc.txt | 6 +- requirements/edx/testing.txt | 6 +- 9 files changed, 455 insertions(+), 168 deletions(-) diff --git a/openedx/features/content_tagging/rest_api/v1/filters.py b/openedx/features/content_tagging/rest_api/v1/filters.py index ee8771d17e..9ad192f545 100644 --- a/openedx/features/content_tagging/rest_api/v1/filters.py +++ b/openedx/features/content_tagging/rest_api/v1/filters.py @@ -4,7 +4,7 @@ API Filters for content tagging org from rest_framework.filters import BaseFilterBackend -from ...rules import is_taxonomy_admin +import openedx_tagging.core.tagging.rules as oel_tagging class UserOrgFilterBackend(BaseFilterBackend): @@ -15,7 +15,7 @@ class UserOrgFilterBackend(BaseFilterBackend): """ def filter_queryset(self, request, queryset, _): - if is_taxonomy_admin(request.user): + if oel_tagging.is_taxonomy_admin(request.user): return queryset return queryset.filter(enabled=True) diff --git a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py index a8177426eb..f2ce3c4064 100644 --- a/openedx/features/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/features/content_tagging/rest_api/v1/tests/test_views.py @@ -7,15 +7,16 @@ from urllib.parse import parse_qs, urlparse import ddt from django.contrib.auth import get_user_model from django.test.testcases import override_settings -from openedx_tagging.core.tagging.models import Taxonomy +from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator +from openedx_tagging.core.tagging.models import Tag, Taxonomy from openedx_tagging.core.tagging.models.system_defined import SystemDefinedTaxonomy from openedx_tagging.core.tagging.rest_api.v1.serializers import TaxonomySerializer from organizations.models import Organization from rest_framework import status from rest_framework.test import APITestCase -from common.djangoapps.student.auth import update_org_role -from common.djangoapps.student.roles import OrgContentCreatorRole +from common.djangoapps.student.auth import add_users, update_org_role +from common.djangoapps.student.roles import CourseStaffRole, OrgContentCreatorRole from openedx.core.djangolib.testing.utils import skip_unless_cms from openedx.features.content_tagging.models import TaxonomyOrg @@ -23,6 +24,7 @@ User = get_user_model() TAXONOMY_ORG_LIST_URL = "/api/content_tagging/v1/taxonomies/" TAXONOMY_ORG_DETAIL_URL = "/api/content_tagging/v1/taxonomies/{pk}/" +OBJECT_TAG_UPDATE_URL = "/api/content_tagging/v1/object_tags/{object_id}/?taxonomy={taxonomy_id}" def check_taxonomy( @@ -52,7 +54,7 @@ def check_taxonomy( assert data["visible_to_authors"] == visible_to_authors -class TestTaxonomyViewSetMixin: +class TestTaxonomyObjectsMixin: """ Sets up data for testing Content Taxonomies. """ @@ -168,7 +170,7 @@ class TestTaxonomyViewSetMixin: @skip_unless_cms @ddt.ddt @override_settings(FEATURES={"ENABLE_CREATOR_GROUP": True}) -class TestTaxonomyViewSet(TestTaxonomyViewSetMixin, APITestCase): +class TestTaxonomyViewSet(TestTaxonomyObjectsMixin, APITestCase): """ Test cases for TaxonomyViewSet when ENABLE_CREATOR_GROUP is True """ @@ -639,3 +641,211 @@ class TestTaxonomyViewSetNoCreatorGroup(TestTaxonomyViewSet): # pylint: disable The permissions are the same for when ENABLED_CREATOR_GRUP is True """ + + +@skip_unless_cms +@ddt.ddt +class TestObjectTagViewSet(TestTaxonomyObjectsMixin, APITestCase): + """ + Testing various cases for the ObjectTagView. + """ + def setUp(self): + """ + Setup the test cases + """ + super().setUp() + self.courseA = CourseLocator("orgA", "101", "test") + self.xblockA = BlockUsageLocator( + course_key=self.courseA, + block_type='problem', + block_id='block_id' + ) + self.courseB = CourseLocator("orgB", "101", "test") + self.xblockB = BlockUsageLocator( + course_key=self.courseB, + block_type='problem', + block_id='block_id' + ) + + self.multiple_taxonomy = Taxonomy.objects.create(name="Multiple Taxonomy", allow_multiple=True) + self.required_taxonomy = Taxonomy.objects.create(name="Required Taxonomy", required=True) + for i in range(20): + # Valid ObjectTags + Tag.objects.create(taxonomy=self.tA1, value=f"Tag {i}") + Tag.objects.create(taxonomy=self.tA2, value=f"Tag {i}") + Tag.objects.create(taxonomy=self.multiple_taxonomy, value=f"Tag {i}") + Tag.objects.create(taxonomy=self.required_taxonomy, value=f"Tag {i}") + + self.open_taxonomy = Taxonomy.objects.create(name="Enabled Free-Text Taxonomy", allow_free_text=True) + + # Add org permissions to taxonomy + TaxonomyOrg.objects.create( + taxonomy=self.multiple_taxonomy, + org=self.orgA, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + TaxonomyOrg.objects.create( + taxonomy=self.required_taxonomy, + org=self.orgA, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + TaxonomyOrg.objects.create( + taxonomy=self.open_taxonomy, + org=self.orgA, + rel_type=TaxonomyOrg.RelType.OWNER, + ) + + add_users(self.userS, CourseStaffRole(self.courseA), self.userA) + + @ddt.data( + # userA and userS are staff in courseA and can tag using enabled taxonomies + (None, "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("user", "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("userA", "tA1", ["Tag 1"], status.HTTP_200_OK), + ("userS", "tA1", ["Tag 1"], status.HTTP_200_OK), + (None, "tA1", [], status.HTTP_403_FORBIDDEN), + ("user", "tA1", [], status.HTTP_403_FORBIDDEN), + ("userA", "tA1", [], status.HTTP_200_OK), + ("userS", "tA1", [], status.HTTP_200_OK), + (None, "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), + ("user", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), + ("userA", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + ("userS", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + (None, "open_taxonomy", ["tag1"], status.HTTP_403_FORBIDDEN), + ("user", "open_taxonomy", ["tag1"], status.HTTP_403_FORBIDDEN), + ("userA", "open_taxonomy", ["tag1"], status.HTTP_200_OK), + ("userS", "open_taxonomy", ["tag1"], status.HTTP_200_OK), + # Only userS is Tagging Admin and can tag objects using disabled taxonomies + (None, "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("user", "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("userA", "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("userS", "tA2", ["Tag 1"], status.HTTP_200_OK), + ) + @ddt.unpack + def test_tag_course(self, user_attr, taxonomy_attr, tag_values, expected_status): + if user_attr: + 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.courseA, 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): + assert len(response.data.get("results")) == len(tag_values) + assert set(t["value"] for t in response.data["results"]) == set(tag_values) + + @ddt.data( + # Can't add invalid tags to a object using a closed taxonomy + (None, "tA1", ["invalid"], status.HTTP_403_FORBIDDEN), + ("user", "tA1", ["invalid"], status.HTTP_403_FORBIDDEN), + ("userA", "tA1", ["invalid"], status.HTTP_400_BAD_REQUEST), + ("userS", "tA1", ["invalid"], status.HTTP_400_BAD_REQUEST), + (None, "multiple_taxonomy", ["invalid"], status.HTTP_403_FORBIDDEN), + ("user", "multiple_taxonomy", ["invalid"], status.HTTP_403_FORBIDDEN), + ("userA", "multiple_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), + ("userS", "multiple_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), + # Staff can't add invalid tags to a object using a closed taxonomy + ("userS", "tA2", ["invalid"], status.HTTP_400_BAD_REQUEST), + ) + @ddt.unpack + def test_tag_course_invalid(self, user_attr, taxonomy_attr, tag_values, expected_status): + if user_attr: + 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.courseA, taxonomy_id=taxonomy.pk) + + response = self.client.put(url, {"tags": tag_values}, format="json") + assert response.status_code == expected_status + assert not status.is_success(expected_status) # No success cases here + + @ddt.data( + # userA and userS are staff in courseA (owner of xblockA) and can tag using enabled taxonomies + (None, "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("user", "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("userA", "tA1", ["Tag 1"], status.HTTP_200_OK), + ("userS", "tA1", ["Tag 1"], status.HTTP_200_OK), + (None, "tA1", [], status.HTTP_403_FORBIDDEN), + ("user", "tA1", [], status.HTTP_403_FORBIDDEN), + ("userA", "tA1", [], status.HTTP_200_OK), + ("userS", "tA1", [], status.HTTP_200_OK), + (None, "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), + ("user", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), + ("userA", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + ("userS", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + (None, "open_taxonomy", ["tag1"], status.HTTP_403_FORBIDDEN), + ("user", "open_taxonomy", ["tag1"], status.HTTP_403_FORBIDDEN), + ("userA", "open_taxonomy", ["tag1"], status.HTTP_200_OK), + ("userS", "open_taxonomy", ["tag1"], status.HTTP_200_OK), + # Only userS is Tagging Admin and can tag objects using disabled taxonomies + (None, "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("user", "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("userA", "tA2", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("userS", "tA2", ["Tag 1"], status.HTTP_200_OK), + ) + @ddt.unpack + def test_tag_xblock(self, user_attr, taxonomy_attr, tag_values, expected_status): + if user_attr: + 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.xblockA, 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): + assert len(response.data.get("results")) == len(tag_values) + assert set(t["value"] for t in response.data["results"]) == set(tag_values) + + @ddt.data( + # Can't add invalid tags to a object using a closed taxonomy + (None, "tA1", ["invalid"], status.HTTP_403_FORBIDDEN), + ("user", "tA1", ["invalid"], status.HTTP_403_FORBIDDEN), + ("userA", "tA1", ["invalid"], status.HTTP_400_BAD_REQUEST), + ("userS", "tA1", ["invalid"], status.HTTP_400_BAD_REQUEST), + (None, "multiple_taxonomy", ["invalid"], status.HTTP_403_FORBIDDEN), + ("user", "multiple_taxonomy", ["invalid"], status.HTTP_403_FORBIDDEN), + ("userA", "multiple_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), + ("userS", "multiple_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), + # Staff can't add invalid tags to a object using a closed taxonomy + ("userS", "tA2", ["invalid"], status.HTTP_400_BAD_REQUEST), + ) + @ddt.unpack + def test_tag_xblock_invalid(self, user_attr, taxonomy_attr, tag_values, expected_status): + if user_attr: + 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.xblockA, taxonomy_id=taxonomy.pk) + + response = self.client.put(url, {"tags": tag_values}, format="json") + assert response.status_code == expected_status + assert not status.is_success(expected_status) # No success cases here + + @ddt.data( + "courseB", + "xblockB", + ) + def test_tag_unauthorized(self, objectid_attr): + """ + Test that a user without access to courseB can't apply tags to it + """ + self.client.force_authenticate(user=self.userA) + object_id = getattr(self, objectid_attr) + + url = OBJECT_TAG_UPDATE_URL.format(object_id=object_id, taxonomy_id=self.tA1.pk) + + response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") + + assert response.status_code == status.HTTP_403_FORBIDDEN diff --git a/openedx/features/content_tagging/rules.py b/openedx/features/content_tagging/rules.py index df256b9331..bad38019ce 100644 --- a/openedx/features/content_tagging/rules.py +++ b/openedx/features/content_tagging/rules.py @@ -1,30 +1,33 @@ """Django rules-based permissions for tagging""" +from __future__ import annotations + +from typing import Union + +import django.contrib.auth.models import openedx_tagging.core.tagging.rules as oel_tagging import rules -from django.contrib.auth import get_user_model +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey, UsageKey -from common.djangoapps.student.auth import is_content_creator +from common.djangoapps.student.auth import is_content_creator, has_studio_write_access from .models import TaxonomyOrg -User = get_user_model() +UserType = Union[django.contrib.auth.models.User, django.contrib.auth.models.AnonymousUser] -def is_taxonomy_user(user: User, taxonomy: oel_tagging.Taxonomy = None) -> bool: +def is_taxonomy_user(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: """ Returns True if the given user is a Taxonomy User for the given content taxonomy. Taxonomy users include global staff and superusers, plus course creators who can create courses for any org. - Otherwise, we need a taxonomy provided to determine if the user is an org-level course creator for one of the - orgs allowed to use this taxonomy. + Otherwise, we need to check taxonomy provided to determine if the user is an org-level course creator for one of + the orgs allowed to use this taxonomy. Only global staff and superusers can use disabled system taxonomies. """ if oel_tagging.is_taxonomy_admin(user): return True - if not taxonomy: - return is_content_creator(user, None) - taxonomy_orgs = TaxonomyOrg.get_organizations( taxonomy=taxonomy, rel_type=TaxonomyOrg.RelType.OWNER, @@ -35,52 +38,35 @@ def is_taxonomy_user(user: User, taxonomy: oel_tagging.Taxonomy = None) -> bool: return False -def is_taxonomy_admin(user: User) -> bool: +@rules.predicate +def can_change_object_tag_objectid(user: UserType, object_id: str) -> bool: """ - Returns True if the given user is a Taxonomy Admin. + Everyone that has permission to edit the object should be able to tag it. + """ + 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) - Taxonomy Admins include global staff and superusers. - """ - return oel_tagging.is_taxonomy_admin(user) + return has_studio_write_access(user, course_key) @rules.predicate -def can_view_taxonomy(user: User, taxonomy: oel_tagging.Taxonomy = None) -> bool: +def can_change_object_tag_taxonomy(user: UserType, taxonomy: oel_tagging.Taxonomy) -> bool: """ - Everyone can potentially view a taxonomy (taxonomy=None). The object permission must be checked - to determine if the user can view a specific taxonomy. - Only taxonomy admins can view a disabled taxonomy. + Taxonomy users can tag objects using tags from any taxonomy that they have permission to view. Only taxonomy admins + can tag objects using tags from disabled taxonomies. """ - if not taxonomy: - return True - - taxonomy = taxonomy.cast() - - return taxonomy.enabled or is_taxonomy_admin(user) + return oel_tagging.is_taxonomy_admin(user) or (taxonomy.cast().enabled and is_taxonomy_user(user, taxonomy)) @rules.predicate -def can_add_taxonomy(user: User) -> bool: - """ - Only taxonomy admins can add taxonomies. - """ - return is_taxonomy_admin(user) - - -@rules.predicate -def can_change_taxonomy(user: User, taxonomy: oel_tagging.Taxonomy = None) -> bool: - """ - Only taxonomy admins can change a taxonomies. - Even taxonomy admins cannot change system taxonomies. - """ - if taxonomy: - taxonomy = taxonomy.cast() - - return (not taxonomy or (not taxonomy.system_defined)) and is_taxonomy_admin(user) - - -@rules.predicate -def can_change_taxonomy_tag(user: User, tag: oel_tagging.Tag = None) -> bool: +def can_change_taxonomy_tag(user: UserType, tag: oel_tagging.Tag | None = None) -> bool: """ Even taxonomy admins cannot add tags to system taxonomies (their tags are system-defined), or free-text taxonomies (these don't have predefined tags). @@ -88,31 +74,18 @@ def can_change_taxonomy_tag(user: User, tag: oel_tagging.Tag = None) -> bool: taxonomy = tag.taxonomy if tag else None if taxonomy: taxonomy = taxonomy.cast() - return is_taxonomy_admin(user) and ( + return oel_tagging.is_taxonomy_admin(user) and ( not tag or not taxonomy or (taxonomy and not taxonomy.allow_free_text and not taxonomy.system_defined) ) -@rules.predicate -def can_change_object_tag(user: User, object_tag: oel_tagging.ObjectTag = None) -> bool: - """ - Taxonomy users can create or modify object tags on enabled taxonomies. - """ - taxonomy = object_tag.taxonomy if object_tag else None - if taxonomy: - taxonomy = taxonomy.cast() - return is_taxonomy_user(user, taxonomy) and ( - not object_tag or not taxonomy or (taxonomy and taxonomy.cast().enabled) - ) - - # Taxonomy -rules.set_perm("oel_tagging.add_taxonomy", can_add_taxonomy) -rules.set_perm("oel_tagging.change_taxonomy", can_change_taxonomy) -rules.set_perm("oel_tagging.delete_taxonomy", can_change_taxonomy) -rules.set_perm("oel_tagging.view_taxonomy", can_view_taxonomy) +rules.set_perm("oel_tagging.add_taxonomy", oel_tagging.is_taxonomy_admin) +rules.set_perm("oel_tagging.change_taxonomy", oel_tagging.can_change_taxonomy) +rules.set_perm("oel_tagging.delete_taxonomy", oel_tagging.can_change_taxonomy) +rules.set_perm("oel_tagging.view_taxonomy", oel_tagging.can_view_taxonomy) # Tag rules.set_perm("oel_tagging.add_tag", can_change_taxonomy_tag) @@ -121,7 +94,12 @@ rules.set_perm("oel_tagging.delete_tag", can_change_taxonomy_tag) rules.set_perm("oel_tagging.view_tag", rules.always_allow) # ObjectTag -rules.set_perm("oel_tagging.add_object_tag", can_change_object_tag) -rules.set_perm("oel_tagging.change_object_tag", can_change_object_tag) -rules.set_perm("oel_tagging.delete_object_tag", can_change_object_tag) +rules.set_perm("oel_tagging.add_object_tag", oel_tagging.can_change_object_tag) +rules.set_perm("oel_tagging.change_object_tag", oel_tagging.can_change_object_tag) +rules.set_perm("oel_tagging.delete_object_tag", oel_tagging.can_change_object_tag) rules.set_perm("oel_tagging.view_object_tag", rules.always_allow) + +# 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. +rules.set_perm("oel_tagging.change_objecttag_taxonomy", can_change_object_tag_taxonomy) +rules.set_perm("oel_tagging.change_objecttag_objectid", can_change_object_tag_objectid) diff --git a/openedx/features/content_tagging/tests/test_rules.py b/openedx/features/content_tagging/tests/test_rules.py index 0bb0e53815..442fda5a71 100644 --- a/openedx/features/content_tagging/tests/test_rules.py +++ b/openedx/features/content_tagging/tests/test_rules.py @@ -3,15 +3,17 @@ import ddt from django.contrib.auth import get_user_model from django.test.testcases import TestCase, override_settings +from opaque_keys import InvalidKeyError +from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator from openedx_tagging.core.tagging.models import ( - ObjectTag, Tag, UserSystemDefinedTaxonomy, ) +from openedx_tagging.core.tagging.rules import ChangeObjectTagPermissionItem from organizations.models import Organization from common.djangoapps.student.auth import add_users, update_org_role -from common.djangoapps.student.roles import CourseCreatorRole, OrgContentCreatorRole +from common.djangoapps.student.roles import CourseCreatorRole, CourseStaffRole, OrgContentCreatorRole from .. import api from .test_api import TestTaxonomyMixin @@ -74,6 +76,115 @@ class TestRulesTaxonomy(TestTaxonomyMixin, TestCase): email="learner@example.com", ) + self.course1 = CourseLocator(self.org1.short_name, "DemoX", "Demo_Course") + self.course2 = CourseLocator(self.org2.short_name, "DemoX", "Demo_Course") + self.courseC = CourseLocator("orgC", "DemoX", "Demo_Course") + + self.xblock1 = BlockUsageLocator( + course_key=self.course1, + block_type='problem', + block_id='block_id' + ) + self.xblock2 = BlockUsageLocator( + course_key=self.course2, + block_type='problem', + block_id='block_id' + ) + self.xblockC = BlockUsageLocator( + course_key=self.courseC, + block_type='problem', + block_id='block_id' + ) + + add_users(self.staff, CourseStaffRole(self.course1), self.user_all_orgs) + add_users(self.staff, CourseStaffRole(self.course1), self.user_both_orgs) + add_users(self.staff, CourseStaffRole(self.course2), self.user_all_orgs) + add_users(self.staff, CourseStaffRole(self.course2), self.user_both_orgs) + add_users(self.staff, CourseStaffRole(self.course2), self.user_org2) + add_users(self.staff, CourseStaffRole(self.course2), self.user_org2) + + self.tax_all_course1 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_all_orgs, + object_id=str(self.course1), + ) + self.tax_all_course2 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_all_orgs, + object_id=str(self.course2), + ) + self.tax_all_xblock1 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_all_orgs, + object_id=str(self.xblock1), + ) + self.tax_all_xblock2 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_all_orgs, + object_id=str(self.xblock2), + ) + + self.tax_both_course1 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_both_orgs, + object_id=str(self.course1), + ) + self.tax_both_course2 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_both_orgs, + object_id=str(self.course2), + ) + self.tax_both_xblock1 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_both_orgs, + object_id=str(self.xblock1), + ) + self.tax_both_xblock2 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_both_orgs, + object_id=str(self.xblock2), + ) + + self.tax1_course1 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_one_org, + object_id=str(self.course1), + ) + self.tax1_xblock1 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_one_org, + object_id=str(self.xblock1), + ) + + self.tax_no_org_course1 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_no_orgs, + object_id=str(self.course1), + ) + + self.tax_no_org_xblock1 = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_no_orgs, + object_id=str(self.xblock1), + ) + + self.disabled_course_tag_perm = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_disabled, + object_id=str(self.course2), + ) + + self.all_orgs_invalid_tag_perm = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_all_orgs, + object_id="course-v1_OpenedX_DemoX_Demo_Course", + ) + self.one_org_invalid_org_tag_perm = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_one_org, + object_id="block-v1_OeX_DemoX_Demo_Course_type_html_block@abcde", + ) + self.no_orgs_invalid_tag_perm = ChangeObjectTagPermissionItem( + taxonomy=self.taxonomy_no_orgs, + object_id=str(self.course1), + ) + + self.all_org_perms = ( + self.tax_all_course1, + self.tax_all_course2, + self.tax_all_xblock1, + self.tax_all_xblock2, + self.tax_both_course1, + self.tax_both_course2, + self.tax_both_xblock1, + self.tax_both_xblock2, + ) + def _expected_users_have_perm( self, perm, obj, learner_perm=False, learner_obj=False, user_org2=True ): @@ -362,25 +473,28 @@ class TestRulesTaxonomy(TestTaxonomyMixin, TestCase): # ObjectTag @ddt.data( - ("oel_tagging.add_object_tag", "disabled_course_tag"), - ("oel_tagging.change_object_tag", "disabled_course_tag"), - ("oel_tagging.delete_object_tag", "disabled_course_tag"), + ("oel_tagging.add_object_tag", "disabled_course_tag_perm"), + ("oel_tagging.change_object_tag", "disabled_course_tag_perm"), + ("oel_tagging.delete_object_tag", "disabled_course_tag_perm"), ) @ddt.unpack def test_object_tag_disabled_taxonomy(self, perm, tag_attr): - """Taxonomy administrators cannot create/edit an ObjectTag with a disabled 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 not self.user_all_orgs.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) + """Only taxonomy administrators can create/edit an ObjectTag using a disabled Taxonomy""" + object_tag_perm = getattr(self, tag_attr) + assert self.superuser.has_perm(perm, object_tag_perm) + assert self.staff.has_perm(perm, object_tag_perm) + assert not self.user_all_orgs.has_perm(perm, object_tag_perm) + assert not self.user_both_orgs.has_perm(perm, object_tag_perm) + assert not self.user_org2.has_perm(perm, object_tag_perm) + assert not self.learner.has_perm(perm, object_tag_perm) @ddt.data( - ("oel_tagging.add_object_tag", "no_orgs_invalid_tag"), - ("oel_tagging.change_object_tag", "no_orgs_invalid_tag"), - ("oel_tagging.delete_object_tag", "no_orgs_invalid_tag"), + ("oel_tagging.add_object_tag", "tax_no_org_course1"), + ("oel_tagging.add_object_tag", "tax_no_org_xblock1"), + ("oel_tagging.change_object_tag", "tax_no_org_course1"), + ("oel_tagging.change_object_tag", "tax_no_org_xblock1"), + ("oel_tagging.delete_object_tag", "tax_no_org_xblock1"), + ("oel_tagging.delete_object_tag", "tax_no_org_course1"), ) @ddt.unpack def test_object_tag_no_orgs(self, perm, tag_attr): @@ -393,62 +507,57 @@ class TestRulesTaxonomy(TestTaxonomyMixin, TestCase): assert not self.user_org2.has_perm(perm, object_tag) assert not self.learner.has_perm(perm, object_tag) - @ddt.data( - ("oel_tagging.add_object_tag", "all_orgs_course_tag"), - ("oel_tagging.add_object_tag", "all_orgs_block_tag"), - ("oel_tagging.add_object_tag", "both_orgs_course_tag"), - ("oel_tagging.add_object_tag", "both_orgs_block_tag"), - ("oel_tagging.add_object_tag", "all_orgs_invalid_tag"), - ("oel_tagging.change_object_tag", "all_orgs_course_tag"), - ("oel_tagging.change_object_tag", "all_orgs_block_tag"), - ("oel_tagging.change_object_tag", "both_orgs_course_tag"), - ("oel_tagging.change_object_tag", "both_orgs_block_tag"), - ("oel_tagging.change_object_tag", "all_orgs_invalid_tag"), - ("oel_tagging.delete_object_tag", "all_orgs_course_tag"), - ("oel_tagging.delete_object_tag", "all_orgs_block_tag"), - ("oel_tagging.delete_object_tag", "both_orgs_course_tag"), - ("oel_tagging.delete_object_tag", "both_orgs_block_tag"), - ("oel_tagging.delete_object_tag", "all_orgs_invalid_tag"), - ) - @ddt.unpack - def test_change_object_tag_all_orgs(self, perm, tag_attr): - """Taxonomy administrators can create/edit an ObjectTag on taxonomies in their org.""" - object_tag = getattr(self, tag_attr) - self._expected_users_have_perm(perm, object_tag) - - @ddt.data( - ("oel_tagging.add_object_tag", "one_org_block_tag"), - ("oel_tagging.add_object_tag", "one_org_invalid_org_tag"), - ("oel_tagging.change_object_tag", "one_org_block_tag"), - ("oel_tagging.change_object_tag", "one_org_invalid_org_tag"), - ("oel_tagging.delete_object_tag", "one_org_block_tag"), - ("oel_tagging.delete_object_tag", "one_org_invalid_org_tag"), - ) - @ddt.unpack - def test_change_object_tag_org1(self, perm, tag_attr): - """Taxonomy administrators can create/edit an ObjectTag on taxonomies in their org.""" - object_tag = getattr(self, tag_attr) - self._expected_users_have_perm(perm, object_tag, user_org2=False) - @ddt.data( "oel_tagging.add_object_tag", "oel_tagging.change_object_tag", "oel_tagging.delete_object_tag", ) - def test_object_tag_no_taxonomy(self, perm): - """Taxonomy administrators can modify an ObjectTag with no Taxonomy""" - object_tag = ObjectTag() + def test_change_object_tag_all_orgs(self, perm): + """ + Taxonomy administrators 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: + assert self.superuser.has_perm(perm, perm_item) + assert self.staff.has_perm(perm, perm_item) + assert self.user_all_orgs.has_perm(perm, perm_item) + assert self.user_both_orgs.has_perm(perm, perm_item) + assert self.user_org2.has_perm(perm, perm_item) == (self.org2.short_name in perm_item.object_id) + assert not self.learner.has_perm(perm, perm_item) - # Global Taxonomy Admins can do pretty much anything - assert self.superuser.has_perm(perm, object_tag) - assert self.staff.has_perm(perm, object_tag) - assert self.user_all_orgs.has_perm(perm, object_tag) + @ddt.data( + ("oel_tagging.add_object_tag", "tax1_course1"), + ("oel_tagging.add_object_tag", "tax1_xblock1"), + ("oel_tagging.change_object_tag", "tax1_course1"), + ("oel_tagging.change_object_tag", "tax1_xblock1"), + ("oel_tagging.delete_object_tag", "tax1_course1"), + ("oel_tagging.delete_object_tag", "tax1_xblock1"), + ) + @ddt.unpack + def test_change_object_tag_org1(self, perm, tag_attr): + """Taxonomy administrators can create/edit an ObjectTag on taxonomies in their org.""" + perm_item = getattr(self, tag_attr) + assert self.superuser.has_perm(perm, perm_item) + assert self.staff.has_perm(perm, perm_item) + assert self.user_all_orgs.has_perm(perm, perm_item) + assert self.user_both_orgs.has_perm(perm, perm_item) + assert not self.user_org2.has_perm(perm, perm_item) + assert not self.learner.has_perm(perm, perm_item) - # Org content creators are bound by a taxonomy's org restrictions, - # so if there's no taxonomy, they can't do anything to it. - 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) + @ddt.data( + + ("oel_tagging.add_object_tag", "one_org_invalid_org_tag_perm"), + ("oel_tagging.add_object_tag", "all_orgs_invalid_tag_perm"), + ("oel_tagging.change_object_tag", "one_org_invalid_org_tag_perm"), + ("oel_tagging.change_object_tag", "all_orgs_invalid_tag_perm"), + ("oel_tagging.delete_object_tag", "one_org_invalid_org_tag_perm"), + ("oel_tagging.delete_object_tag", "all_orgs_invalid_tag_perm"), + ) + @ddt.unpack + def test_change_object_tag_invalid_key(self, perm, tag_attr): + perm_item = getattr(self, tag_attr) + with self.assertRaises(InvalidKeyError): + assert self.staff.has_perm(perm, perm_item) @ddt.data( "all_orgs_course_tag", @@ -493,31 +602,11 @@ class TestRulesTaxonomyNoCreatorGroup( super()._expected_users_have_perm( perm=perm, obj=obj, - learner_perm=True, - learner_obj=True, - user_org2=True, + learner_perm=learner_perm, + learner_obj=learner_obj, + user_org2=user_org2, ) - @ddt.data( - "oel_tagging.add_object_tag", - "oel_tagging.change_object_tag", - "oel_tagging.delete_object_tag", - ) - def test_object_tag_no_taxonomy(self, perm): - """Taxonomy administrators can modify an ObjectTag with no Taxonomy""" - object_tag = ObjectTag() - - # Global Taxonomy Admins can do pretty much anything - assert self.superuser.has_perm(perm, object_tag) - assert self.staff.has_perm(perm, object_tag) - assert self.user_all_orgs.has_perm(perm, object_tag) - - # Org content creators are bound by a taxonomy's org restrictions, - # but since there's no org restrictions enabled, anyone has these permissions. - assert self.user_both_orgs.has_perm(perm, object_tag) - assert self.user_org2.has_perm(perm, object_tag) - assert self.learner.has_perm(perm, object_tag) - # Taxonomy @ddt.data( diff --git a/requirements/constraints.txt b/requirements/constraints.txt index c505a91a38..84875f9ac0 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -126,3 +126,6 @@ click==8.1.6 # openedx-events 8.6.0 introduces publishing via configuration. Ticket to unpin: https://github.com/edx/edx-arch-experiments/issues/381 openedx-events<8.6.0 # Open edX Events from Hooks Extension Framework (OEP-50) + +# pinning this version to avoid updates while the library is being developed +openedx-learning==0.1.6 \ No newline at end of file diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index ed183c448c..83cab19947 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -779,8 +779,10 @@ openedx-filters==1.6.0 # via # -r requirements/edx/kernel.in # lti-consumer-xblock -openedx-learning==0.1.5 - # via -r requirements/edx/kernel.in +openedx-learning==0.1.6 + # via + # -c requirements/edx/../constraints.txt + # -r requirements/edx/kernel.in openedx-mongodbproxy==0.2.0 # via -r requirements/edx/kernel.in optimizely-sdk==4.1.1 diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index eff77b33e4..5f919bbf36 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1311,8 +1311,9 @@ openedx-filters==1.6.0 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # lti-consumer-xblock -openedx-learning==0.1.5 +openedx-learning==0.1.6 # via + # -c requirements/edx/../constraints.txt # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt openedx-mongodbproxy==0.2.0 diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 8019ea9092..a5b382bde3 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -919,8 +919,10 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.1.5 - # via -r requirements/edx/base.txt +openedx-learning==0.1.6 + # via + # -c requirements/edx/../constraints.txt + # -r requirements/edx/base.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.txt optimizely-sdk==4.1.1 diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 4769cca690..91806e60f0 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -989,8 +989,10 @@ openedx-filters==1.6.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock -openedx-learning==0.1.5 - # via -r requirements/edx/base.txt +openedx-learning==0.1.6 + # via + # -c requirements/edx/../constraints.txt + # -r requirements/edx/base.txt openedx-mongodbproxy==0.2.0 # via -r requirements/edx/base.txt optimizely-sdk==4.1.1