From 38da66b74aae97bdfe43397ab706cb6fa8507bc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 16 Oct 2023 16:34:17 -0300 Subject: [PATCH] feat: add "can view object tags" permissions (#94) --- openedx_learning/__init__.py | 2 +- .../core/tagging/rest_api/v1/views.py | 40 +++++++-- openedx_tagging/core/tagging/rules.py | 62 ++++++++++++- .../core/tagging/test_rules.py | 10 +-- .../core/tagging/test_views.py | 87 +++++++++++-------- 5 files changed, 149 insertions(+), 52 deletions(-) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index 19322ba2..d88e37a3 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.2.4" +__version__ = "0.2.5" diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index b4774baf..743f5bcd 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -27,7 +27,7 @@ from ...import_export.api import export_tags from ...import_export.parsers import ParserFormat from ...models import Taxonomy -from ...rules import ChangeObjectTagPermissionItem +from ...rules import ObjectTagPermissionItem from ..paginators import SEARCH_TAGS_THRESHOLD, TAGS_THRESHOLD, DisabledTagsPagination, TagsPagination from .permissions import ObjectTagObjectPermissions, TagListPermissions, TaxonomyObjectPermissions from .serializers import ( @@ -298,10 +298,30 @@ def get_queryset(self) -> models.QuerySet: data=self.request.query_params.dict() ) query_params.is_valid(raise_exception=True) - taxonomy_id = query_params.data.get("taxonomy", None) + taxonomy = query_params.validated_data.get("taxonomy", None) + taxonomy_id = None + if taxonomy: + taxonomy = taxonomy.cast() + taxonomy_id = taxonomy.id + + perm = "oel_tagging.view_objecttag" + perm_obj = ObjectTagPermissionItem( + taxonomy=taxonomy, + object_id=object_id, + ) + + if not self.request.user.has_perm( + perm, + # The obj arg expects a model, but we are passing an object + perm_obj, # type: ignore[arg-type] + ): + raise PermissionDenied( + "You do not have permission to view object tags for this taxonomy or object_id." + ) + return get_object_tags(object_id, taxonomy_id) - def retrieve(self, request, *args, **kwargs): + def retrieve(self, request, *args, **kwargs) -> Response: """ Retrieve ObjectTags that belong to a given object_id @@ -312,11 +332,11 @@ def retrieve(self, request, *args, **kwargs): path and returns a it as a single result however that is not behavior we want. """ - object_tags = self.get_queryset() + object_tags = self.filter_queryset(self.get_queryset()) serializer = ObjectTagSerializer(object_tags, many=True) return Response(serializer.data) - def update(self, request, *args, **kwargs): + def update(self, request, *args, **kwargs) -> Response: """ Update ObjectTags that belong to a given object_id @@ -352,15 +372,19 @@ def update(self, request, *args, **kwargs): taxonomy = query_params.validated_data.get("taxonomy", None) taxonomy = taxonomy.cast() - perm = f"{taxonomy._meta.app_label}.change_objecttag" + perm = "oel_tagging.change_objecttag" object_id = kwargs.pop('object_id') - perm_obj = ChangeObjectTagPermissionItem( + perm_obj = ObjectTagPermissionItem( taxonomy=taxonomy, object_id=object_id, ) - if not request.user.has_perm(perm, perm_obj): + if not request.user.has_perm( + perm, + # The obj arg expects a model, but we are passing an object + perm_obj, # type: ignore[arg-type] + ): raise PermissionDenied( "You do not have permission to change object tags for this taxonomy or object_id." ) diff --git a/openedx_tagging/core/tagging/rules.py b/openedx_tagging/core/tagging/rules.py index 878b1c90..8249628f 100644 --- a/openedx_tagging/core/tagging/rules.py +++ b/openedx_tagging/core/tagging/rules.py @@ -23,7 +23,7 @@ @define -class ChangeObjectTagPermissionItem: +class ObjectTagPermissionItem: """ Pair of taxonomy and object_id used for permission checking. """ @@ -65,6 +65,58 @@ def can_change_tag(user: UserType, tag: Tag | None = None) -> bool: ) +@rules.predicate +def can_view_object_tag_taxonomy(user: UserType, taxonomy: Taxonomy) -> bool: + """ + Only enabled taxonomy and users with permission to view this taxonomy can view object tags + from that taxonomy. + + This rule is different from can_view_taxonomy because it checks if the taxonomy is enabled. + """ + if not taxonomy: + return True + + return taxonomy.cast().enabled and can_view_taxonomy(user, taxonomy) + + +@rules.predicate +def can_view_object_tag_objectid(_user: UserType, _object_id: str) -> bool: + """ + Everybody can view object tags from any objects. + + This rule could be defined in other apps for proper permission checking. + """ + return True + + +@rules.predicate +def can_view_object_tag( + user: UserType, perm_obj: ObjectTagPermissionItem | None = None +) -> bool: + """ + Checks if the user has permissions to view tags on the given taxonomy and object_id. + """ + + # The following code allows METHOD permission (GET) in the viewset for everyone + if perm_obj is None: + return True + + # Checks the permission for the taxonomy + taxonomy_perm = user.has_perm( + "oel_tagging.view_objecttag_taxonomy", perm_obj.taxonomy + ) + if not taxonomy_perm: + return False + + # Checks the permission for the object_id + objectid_perm = user.has_perm( + "oel_tagging.view_objecttag_objectid", + # The obj arg expects an object, but we are passing a string + perm_obj.object_id, # type: ignore[arg-type] + ) + return objectid_perm + + @rules.predicate def can_change_object_tag_objectid(_user: UserType, _object_id: str) -> bool: """ @@ -77,7 +129,7 @@ def can_change_object_tag_objectid(_user: UserType, _object_id: str) -> bool: @rules.predicate def can_change_object_tag( - user: UserType, perm_obj: ChangeObjectTagPermissionItem | None = None + user: UserType, perm_obj: ObjectTagPermissionItem | None = None ) -> bool: """ Checks if the user has permissions to create or modify tags on the given taxonomy and object_id. @@ -122,8 +174,10 @@ def can_change_object_tag( rules.add_perm("oel_tagging.add_objecttag", can_change_object_tag) rules.add_perm("oel_tagging.change_objecttag", can_change_object_tag) rules.add_perm("oel_tagging.delete_objecttag", can_change_object_tag) -rules.add_perm("oel_tagging.view_objecttag", rules.always_allow) +rules.add_perm("oel_tagging.view_objecttag", can_view_object_tag) # Users can tag objects using tags from any taxonomy that they have permission to view -rules.add_perm("oel_tagging.change_objecttag_taxonomy", can_view_taxonomy) +rules.add_perm("oel_tagging.view_objecttag_objectid", can_view_object_tag_objectid) +rules.add_perm("oel_tagging.view_objecttag_taxonomy", can_view_object_tag_taxonomy) +rules.add_perm("oel_tagging.change_objecttag_taxonomy", can_view_object_tag_taxonomy) rules.add_perm("oel_tagging.change_objecttag_objectid", can_change_object_tag_objectid) diff --git a/tests/openedx_tagging/core/tagging/test_rules.py b/tests/openedx_tagging/core/tagging/test_rules.py index d49f8b99..f30cbd27 100644 --- a/tests/openedx_tagging/core/tagging/test_rules.py +++ b/tests/openedx_tagging/core/tagging/test_rules.py @@ -7,7 +7,7 @@ from django.test.testcases import TestCase from openedx_tagging.core.tagging.models import ObjectTag -from openedx_tagging.core.tagging.rules import ChangeObjectTagPermissionItem +from openedx_tagging.core.tagging.rules import ObjectTagPermissionItem from .test_models import TestTagTaxonomyMixin @@ -188,7 +188,7 @@ def test_add_change_object_tag(self, perm): """ Everyone can create/edit an ObjectTag with an enabled Taxonomy """ - obj_perm = ChangeObjectTagPermissionItem( + obj_perm = ObjectTagPermissionItem( taxonomy=self.object_tag.taxonomy, object_id=self.object_tag.object_id, ) @@ -210,12 +210,12 @@ def test_object_tag_disabled_taxonomy(self, perm): """ self.taxonomy.enabled = False self.taxonomy.save() - obj_perm = ChangeObjectTagPermissionItem( + obj_perm = ObjectTagPermissionItem( taxonomy=self.object_tag.taxonomy, object_id=self.object_tag.object_id, ) assert self.superuser.has_perm(perm, obj_perm) - assert self.staff.has_perm(perm, obj_perm) + assert not self.staff.has_perm(perm, obj_perm) assert not self.learner.has_perm(perm, obj_perm) @ddt.data( @@ -229,7 +229,7 @@ def test_object_tag_without_object_permission(self, perm): """ self.taxonomy.enabled = False self.taxonomy.save() - obj_perm = ChangeObjectTagPermissionItem( + obj_perm = ObjectTagPermissionItem( taxonomy=self.object_tag.taxonomy, object_id="not abc", ) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 98f2332b..d97296ba 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -17,6 +17,7 @@ from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy from openedx_tagging.core.tagging.models.system_defined import SystemDefinedTaxonomy from openedx_tagging.core.tagging.rest_api.paginators import TagsPagination +from openedx_tagging.core.tagging.rules import can_change_object_tag_objectid, can_view_object_tag_objectid User = get_user_model() @@ -506,11 +507,23 @@ class TestObjectTagViewSet(APITestCase): def setUp(self): - def _object_permission(_user, object_id: str) -> bool: + def _change_object_permission(user, object_id: str) -> bool: """ - Everyone have object permission on object_id "abc" + For testing, let everyone have edit object permission on object_id "abc" and "limit_tag_count" """ - return object_id in ("abc", "limit_tag_count") + if object_id in ("abc", "limit_tag_count"): + return True + + return can_change_object_tag_objectid(user, object_id) + + def _view_object_permission(user, object_id: str) -> bool: + """ + For testing, let everyone have view object permission on all objects but "unauthorized_id" + """ + if object_id == "unauthorized_id": + return False + + return can_view_object_tag_objectid(user, object_id) super().setUp() @@ -580,22 +593,20 @@ def _object_permission(_user, object_id: str) -> bool: self.dummy_taxonomies.append(taxonomy) # Override the object permission for the test - rules.set_perm("oel_tagging.change_objecttag_objectid", _object_permission) + rules.set_perm("oel_tagging.change_objecttag_objectid", _change_object_permission) + rules.set_perm("oel_tagging.view_objecttag_objectid", _view_object_permission) @ddt.data( - (None, "abc", status.HTTP_401_UNAUTHORIZED, None), - ("user", "abc", status.HTTP_200_OK, 81), - ("staff", "abc", status.HTTP_200_OK, 81), - (None, "non-existing-id", status.HTTP_401_UNAUTHORIZED, None), - ("user", "non-existing-id", status.HTTP_200_OK, 0), - ("staff", "non-existing-id", status.HTTP_200_OK, 0), + (None, status.HTTP_401_UNAUTHORIZED, None), + ("user", status.HTTP_200_OK, 81), + ("staff", status.HTTP_200_OK, 81), ) @ddt.unpack - def test_retrieve_object_tags(self, user_attr, object_id, expected_status, expected_count): + def test_retrieve_object_tags(self, user_attr, expected_status, expected_count): """ Test retrieving object tags """ - url = OBJECT_TAGS_RETRIEVE_URL.format(object_id=object_id) + url = OBJECT_TAGS_RETRIEVE_URL.format(object_id="abc") if user_attr: user = getattr(self, user_attr) @@ -607,6 +618,15 @@ def test_retrieve_object_tags(self, user_attr, object_id, expected_status, expec if status.is_success(expected_status): assert len(response.data) == expected_count + def test_retrieve_object_tags_unauthorized(self): + """ + Test retrieving object tags from an unauthorized object_id + """ + url = OBJECT_TAGS_RETRIEVE_URL.format(object_id="unauthorized_id") + self.client.force_authenticate(user=self.staff) + response = self.client.get(url) + assert response.status_code == status.HTTP_403_FORBIDDEN + @ddt.data( (None, "abc", status.HTTP_401_UNAUTHORIZED, None), ("user", "abc", status.HTTP_200_OK, 20), @@ -694,30 +714,30 @@ def test_object_tags_remaining_http_methods( assert response.status_code == expected_status @ddt.data( - # Users and staff can add tags to a taxonomy + # Users and staff can add tags (None, "language_taxonomy", ["Portuguese"], status.HTTP_401_UNAUTHORIZED), ("user", "language_taxonomy", ["Portuguese"], status.HTTP_200_OK), ("staff", "language_taxonomy", ["Portuguese"], status.HTTP_200_OK), - # Users and staff can clear add tags to a taxonomy + # Users and staff can clear add tags (None, "enabled_taxonomy", ["Tag 1"], status.HTTP_401_UNAUTHORIZED), ("user", "enabled_taxonomy", ["Tag 1"], status.HTTP_200_OK), ("staff", "enabled_taxonomy", ["Tag 1"], status.HTTP_200_OK), - # Only staff can add tag to a disabled taxonomy + # Nobody can add tag using a disabled taxonomy (None, "disabled_taxonomy", ["Tag 1"], status.HTTP_401_UNAUTHORIZED), ("user", "disabled_taxonomy", ["Tag 1"], status.HTTP_403_FORBIDDEN), - ("staff", "disabled_taxonomy", ["Tag 1"], status.HTTP_200_OK), - # Users and staff can add a single tag to a allow_multiple=True taxonomy + ("staff", "disabled_taxonomy", ["Tag 1"], status.HTTP_403_FORBIDDEN), + # Users and staff can add a single tag using a allow_multiple=True taxonomy (None, "multiple_taxonomy", ["Tag 1"], status.HTTP_401_UNAUTHORIZED), ("user", "multiple_taxonomy", ["Tag 1"], status.HTTP_200_OK), ("staff", "multiple_taxonomy", ["Tag 1"], status.HTTP_200_OK), - # Users and staff can add tags to an open taxonomy + # Users and staff can add tags using an open taxonomy (None, "open_taxonomy_enabled", ["tag1"], status.HTTP_401_UNAUTHORIZED), ("user", "open_taxonomy_enabled", ["tag1"], status.HTTP_200_OK), ("staff", "open_taxonomy_enabled", ["tag1"], status.HTTP_200_OK), - # Only staff can add tags to a disabled open taxonomy + # Nobody can add tags using a disabled open taxonomy (None, "open_taxonomy_disabled", ["tag1"], status.HTTP_401_UNAUTHORIZED), ("user", "open_taxonomy_disabled", ["tag1"], status.HTTP_403_FORBIDDEN), - ("staff", "open_taxonomy_disabled", ["tag1"], status.HTTP_200_OK), + ("staff", "open_taxonomy_disabled", ["tag1"], status.HTTP_403_FORBIDDEN), ) @ddt.unpack def test_tag_object(self, user_attr, taxonomy_attr, tag_values, expected_status): @@ -736,7 +756,7 @@ def test_tag_object(self, user_attr, taxonomy_attr, tag_values, expected_status) assert set(t["value"] for t in response.data) == set(tag_values) @ddt.data( - # Can't add invalid tags to a closed taxonomy + # Can't add invalid tags using a closed taxonomy (None, "language_taxonomy", ["Invalid"], status.HTTP_401_UNAUTHORIZED), ("user", "language_taxonomy", ["Invalid"], status.HTTP_400_BAD_REQUEST), ("staff", "language_taxonomy", ["Invalid"], status.HTTP_400_BAD_REQUEST), @@ -746,10 +766,10 @@ def test_tag_object(self, user_attr, taxonomy_attr, tag_values, expected_status) (None, "multiple_taxonomy", ["invalid"], status.HTTP_401_UNAUTHORIZED), ("user", "multiple_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), ("staff", "multiple_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), - # Users can't edit tags from a disabled taxonomy. Staff can't add invalid tags to a closed taxonomy + # Nobody can edit object tags using a disabled taxonomy. (None, "disabled_taxonomy", ["invalid"], status.HTTP_401_UNAUTHORIZED), ("user", "disabled_taxonomy", ["invalid"], status.HTTP_403_FORBIDDEN), - ("staff", "disabled_taxonomy", ["invalid"], status.HTTP_400_BAD_REQUEST), + ("staff", "disabled_taxonomy", ["invalid"], status.HTTP_403_FORBIDDEN), ) @ddt.unpack def test_tag_object_invalid(self, user_attr, taxonomy_attr, tag_values, expected_status): @@ -766,21 +786,21 @@ def test_tag_object_invalid(self, user_attr, taxonomy_attr, tag_values, expected assert not status.is_success(expected_status) # No success cases here @ddt.data( - # Users and staff can clear tags from a taxonomy + # Users and staff can clear tags (None, "enabled_taxonomy", [], status.HTTP_401_UNAUTHORIZED), ("user", "enabled_taxonomy", [], status.HTTP_200_OK), ("staff", "enabled_taxonomy", [], status.HTTP_200_OK), - # Users and staff can clear tags from a allow_multiple=True taxonomy + # Users and staff can clear object tags using a allow_multiple=True taxonomy (None, "multiple_taxonomy", [], status.HTTP_401_UNAUTHORIZED), ("user", "multiple_taxonomy", [], status.HTTP_200_OK), ("staff", "multiple_taxonomy", [], status.HTTP_200_OK), - # Only staff can clear tags from a disabled taxonomy + # Nobody can clear tags using a disabled taxonomy (None, "disabled_taxonomy", [], status.HTTP_401_UNAUTHORIZED), ("user", "disabled_taxonomy", [], status.HTTP_403_FORBIDDEN), - ("staff", "disabled_taxonomy", [], status.HTTP_200_OK), + ("staff", "disabled_taxonomy", [], status.HTTP_403_FORBIDDEN), (None, "open_taxonomy_disabled", [], status.HTTP_401_UNAUTHORIZED), ("user", "open_taxonomy_disabled", [], status.HTTP_403_FORBIDDEN), - ("staff", "open_taxonomy_disabled", [], status.HTTP_200_OK), + ("staff", "open_taxonomy_disabled", [], status.HTTP_403_FORBIDDEN), ) @ddt.unpack def test_tag_object_clear(self, user_attr, taxonomy_attr, tag_values, expected_status): @@ -799,25 +819,24 @@ def test_tag_object_clear(self, user_attr, taxonomy_attr, tag_values, expected_s assert set(t["value"] for t in response.data) == set(tag_values) @ddt.data( - # Users and staff can add multiple tags to a allow_multiple=True taxonomy + # Users and staff can add multiple tags using a allow_multiple=True taxonomy (None, "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_401_UNAUTHORIZED), ("user", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), ("staff", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), (None, "open_taxonomy_enabled", ["tag1", "tag2"], status.HTTP_401_UNAUTHORIZED), ("user", "open_taxonomy_enabled", ["tag1", "tag2"], status.HTTP_400_BAD_REQUEST), ("staff", "open_taxonomy_enabled", ["tag1", "tag2"], status.HTTP_400_BAD_REQUEST), - # Users and staff can't add multple tags to a allow_multiple=False taxonomy + # Users and staff can't add multple tags using a allow_multiple=False taxonomy (None, "enabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_401_UNAUTHORIZED), ("user", "enabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_400_BAD_REQUEST), ("staff", "enabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_400_BAD_REQUEST), (None, "language_taxonomy", ["Portuguese", "English"], status.HTTP_401_UNAUTHORIZED), ("user", "language_taxonomy", ["Portuguese", "English"], status.HTTP_400_BAD_REQUEST), ("staff", "language_taxonomy", ["Portuguese", "English"], status.HTTP_400_BAD_REQUEST), - # Users can't edit tags from a disabled taxonomy. Staff can't add multiple tags to - # a taxonomy with allow_multiple=False + # Nobody can edit tags using a disabled taxonomy. (None, "disabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_401_UNAUTHORIZED), ("user", "disabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), - ("staff", "disabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_400_BAD_REQUEST), + ("staff", "disabled_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), ) @ddt.unpack def test_tag_object_multiple(self, user_attr, taxonomy_attr, tag_values, expected_status): @@ -846,7 +865,7 @@ def test_tag_object_without_permission(self, user_attr, expected_status): user = getattr(self, user_attr) self.client.force_authenticate(user=user) - url = OBJECT_TAGS_UPDATE_URL.format(object_id="not abc", taxonomy_id=self.enabled_taxonomy.pk) + url = OBJECT_TAGS_UPDATE_URL.format(object_id="view_only", taxonomy_id=self.enabled_taxonomy.pk) response = self.client.put(url, {"tags": ["Tag 1"]}, format="json") assert response.status_code == expected_status