From c1502d01068559e3aae76ad16aa7cea7ef2e13fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Tue, 10 Oct 2023 11:23:42 -0300 Subject: [PATCH 01/10] feat: add ObjectTag view permissions --- openedx_learning/__init__.py | 2 +- .../core/tagging/rest_api/v1/views.py | 14 +++++++++- openedx_tagging/core/tagging/rules.py | 11 ++++++++ .../core/tagging/test_views.py | 28 ++++++++++++++----- 4 files changed, 46 insertions(+), 9 deletions(-) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index fe8603ba..fcfa685c 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.2.2" +__version__ = "0.2.3" diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index f940add1..c7181842 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -242,6 +242,18 @@ def get_queryset(self) -> models.QuerySet: ) query_params.is_valid(raise_exception=True) taxonomy_id = query_params.data.get("taxonomy", None) + + # Checks the permission for the object_id + objectid_perm = self.request.user.has_perm( + "oel_tagging.view_objecttag_objectid", + # The obj arg expects an object, but we are passing a string + object_id, # type: ignore[arg-type] + ) + + if not objectid_perm: + raise PermissionDenied( + "You do not have permission to view object tags for this object_id." + ) return get_object_tags(object_id, taxonomy_id) def retrieve(self, request, *args, **kwargs): @@ -255,7 +267,7 @@ 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) diff --git a/openedx_tagging/core/tagging/rules.py b/openedx_tagging/core/tagging/rules.py index 00ec8811..e9724499 100644 --- a/openedx_tagging/core/tagging/rules.py +++ b/openedx_tagging/core/tagging/rules.py @@ -65,6 +65,16 @@ def can_change_tag(user: UserType, tag: Tag | None = None) -> bool: ) +@rules.predicate +def can_view_object_tag_objectid(_user: UserType, _object_id: str) -> bool: + """ + Nobody can view object tags without checking the permission for the tagged object. + + This rule should be defined in other apps for proper permission checking. + """ + return False + + @rules.predicate def can_change_object_tag_objectid(_user: UserType, _object_id: str) -> bool: """ @@ -124,5 +134,6 @@ def can_change_object_tag( rules.add_perm("oel_tagging.view_objecttag", rules.always_allow) # Users can tag objects using tags from any taxonomy that they have permission to view +rules.add_perm("oel_tagging.view_objecttag_objectid", can_view_object_tag_objectid) rules.add_perm("oel_tagging.change_objecttag_taxonomy", can_view_taxonomy) rules.add_perm("oel_tagging.change_objecttag_objectid", can_change_object_tag_objectid) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 9183ee5f..0650999d 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -15,6 +15,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() @@ -404,11 +405,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" + Everyone have 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: + """ + Everyone have object permission on object_id "abc", "limit_tag_count" and "view_only" + """ + if object_id in ("abc", "limit_tag_count", "view_only"): + return True + + return can_view_object_tag_objectid(user, object_id) super().setUp() @@ -478,15 +491,16 @@ 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_403_FORBIDDEN, None), ("user", "abc", status.HTTP_200_OK, 81), ("staff", "abc", status.HTTP_200_OK, 81), (None, "non-existing-id", status.HTTP_403_FORBIDDEN, None), - ("user", "non-existing-id", status.HTTP_200_OK, 0), - ("staff", "non-existing-id", status.HTTP_200_OK, 0), + ("user", "non-existing-id", status.HTTP_403_FORBIDDEN, None), + ("staff", "non-existing-id", status.HTTP_403_FORBIDDEN, None), ) @ddt.unpack def test_retrieve_object_tags(self, user_attr, object_id, expected_status, expected_count): @@ -744,7 +758,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 From d7f78c1f723542d8ec0694fbc072a9e31e84ddee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 11 Oct 2023 09:21:02 -0300 Subject: [PATCH 02/10] refactor: change can_view_object_tag_objectid to return True --- openedx_tagging/core/tagging/rules.py | 6 +-- .../core/tagging/test_views.py | 37 +++++++++++++------ 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/openedx_tagging/core/tagging/rules.py b/openedx_tagging/core/tagging/rules.py index e9724499..426d80cb 100644 --- a/openedx_tagging/core/tagging/rules.py +++ b/openedx_tagging/core/tagging/rules.py @@ -68,11 +68,11 @@ def can_change_tag(user: UserType, tag: Tag | None = None) -> bool: @rules.predicate def can_view_object_tag_objectid(_user: UserType, _object_id: str) -> bool: """ - Nobody can view object tags without checking the permission for the tagged object. + Everybody can view object tags from any objects. - This rule should be defined in other apps for proper permission checking. + This rule could be defined in other apps for proper permission checking. """ - return False + return True @rules.predicate diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 0650999d..4485a114 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -416,10 +416,10 @@ def _change_object_permission(user, object_id: str) -> bool: def _view_object_permission(user, object_id: str) -> bool: """ - Everyone have object permission on object_id "abc", "limit_tag_count" and "view_only" + Everyone have object permission on all objects but "unauthorized_id" """ - if object_id in ("abc", "limit_tag_count", "view_only"): - return True + if object_id == "unauthorized_id": + return False return can_view_object_tag_objectid(user, object_id) @@ -495,19 +495,16 @@ def _view_object_permission(user, object_id: str) -> bool: rules.set_perm("oel_tagging.view_objecttag_objectid", _view_object_permission) @ddt.data( - (None, "abc", status.HTTP_403_FORBIDDEN, None), - ("user", "abc", status.HTTP_200_OK, 81), - ("staff", "abc", status.HTTP_200_OK, 81), - (None, "non-existing-id", status.HTTP_403_FORBIDDEN, None), - ("user", "non-existing-id", status.HTTP_403_FORBIDDEN, None), - ("staff", "non-existing-id", status.HTTP_403_FORBIDDEN, None), + (None, status.HTTP_403_FORBIDDEN, 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) @@ -519,6 +516,24 @@ 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 + @ddt.data( + None, + "user", + "staff" + ) + def test_retrieve_object_tags_unauthorized(self, user_attr): + """ + Test retrieving object tags from an unauthorized object_id + """ + url = OBJECT_TAGS_RETRIEVE_URL.format(object_id="unauthorized_id") + + if user_attr: + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + + response = self.client.get(url) + assert response.status_code == status.HTTP_403_FORBIDDEN + @ddt.data( (None, "abc", status.HTTP_403_FORBIDDEN, None), ("user", "abc", status.HTTP_200_OK, 20), From 12a6bb365a1d1d8d7310dd20da58e9b5f9dccd03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Wed, 11 Oct 2023 12:50:46 -0300 Subject: [PATCH 03/10] chore: bump version to 0.2.4 --- openedx_learning/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index fcfa685c..19322ba2 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -1,4 +1,4 @@ """ Open edX Learning ("Learning Core"). """ -__version__ = "0.2.3" +__version__ = "0.2.4" From 1796d36d62a40a0293110876d98cbeb8f9fefc28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 12 Oct 2023 13:26:30 -0300 Subject: [PATCH 04/10] fix: update docstring and simplify test_retrieve_object_tags_unauthorized Co-authored-by: Jillian --- .../openedx_tagging/core/tagging/test_views.py | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 4485a114..d5861e71 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -407,7 +407,7 @@ def setUp(self): def _change_object_permission(user, object_id: str) -> bool: """ - Everyone have object permission on object_id "abc" and "limit_tag_count" + For testing, let everyone have edit object permission on object_id "abc" and "limit_tag_count" """ if object_id in ("abc", "limit_tag_count"): return True @@ -416,7 +416,7 @@ def _change_object_permission(user, object_id: str) -> bool: def _view_object_permission(user, object_id: str) -> bool: """ - Everyone have object permission on all objects but "unauthorized_id" + For testing, let everyone have view object permission on all objects but "unauthorized_id" """ if object_id == "unauthorized_id": return False @@ -516,21 +516,12 @@ def test_retrieve_object_tags(self, user_attr, expected_status, expected_count): if status.is_success(expected_status): assert len(response.data) == expected_count - @ddt.data( - None, - "user", - "staff" - ) - def test_retrieve_object_tags_unauthorized(self, user_attr): + 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") - - if user_attr: - user = getattr(self, user_attr) - self.client.force_authenticate(user=user) - + self.client.force_authenticate(user=self.staff) response = self.client.get(url) assert response.status_code == status.HTTP_403_FORBIDDEN From d54fb3a046d02a94e719fe681592e392c7bbfef7 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 12 Oct 2023 11:15:14 +1030 Subject: [PATCH 05/10] refactor: makes oel_tagging.view_objecttag work like change_objecttag The "view objecttag" rule now checks permissions for the taxonomy and object_id if provided. --- .../core/tagging/rest_api/v1/views.py | 24 +++++++------ openedx_tagging/core/tagging/rules.py | 35 +++++++++++++++++-- .../core/tagging/test_rules.py | 8 ++--- 3 files changed, 50 insertions(+), 17 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index c7181842..baf91a67 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -24,7 +24,7 @@ tag_object, ) 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 ( @@ -241,19 +241,23 @@ 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 - # Checks the permission for the object_id - objectid_perm = self.request.user.has_perm( - "oel_tagging.view_objecttag_objectid", - # The obj arg expects an object, but we are passing a string - object_id, # type: ignore[arg-type] + perm = "oel_tagging.view_objecttag" + perm_obj = ObjectTagPermissionItem( + taxonomy=taxonomy, + object_id=object_id, ) - if not objectid_perm: + if not self.request.user.has_perm(perm, perm_obj): raise PermissionDenied( - "You do not have permission to view object tags for this object_id." + "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): @@ -310,7 +314,7 @@ def update(self, request, *args, **kwargs): perm = f"{taxonomy._meta.app_label}.change_objecttag" object_id = kwargs.pop('object_id') - perm_obj = ChangeObjectTagPermissionItem( + perm_obj = ObjectTagPermissionItem( taxonomy=taxonomy, object_id=object_id, ) diff --git a/openedx_tagging/core/tagging/rules.py b/openedx_tagging/core/tagging/rules.py index 426d80cb..c3772553 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. """ @@ -75,6 +75,34 @@ def can_view_object_tag_objectid(_user: UserType, _object_id: str) -> bool: 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: """ @@ -87,7 +115,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. @@ -131,9 +159,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.view_objecttag_objectid", can_view_object_tag_objectid) +rules.add_perm("oel_tagging.view_objecttag_taxonomy", can_view_taxonomy) rules.add_perm("oel_tagging.change_objecttag_taxonomy", can_view_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..66af076f 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,7 +210,7 @@ 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, ) @@ -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", ) From 677260d4f014ebffcc5d96abbffb251d810f505f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 12 Oct 2023 13:57:22 -0300 Subject: [PATCH 06/10] refactor: add typings --- .../core/tagging/rest_api/v1/views.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/openedx_tagging/core/tagging/rest_api/v1/views.py b/openedx_tagging/core/tagging/rest_api/v1/views.py index baf91a67..18e3cec0 100644 --- a/openedx_tagging/core/tagging/rest_api/v1/views.py +++ b/openedx_tagging/core/tagging/rest_api/v1/views.py @@ -253,14 +253,18 @@ def get_queryset(self) -> models.QuerySet: object_id=object_id, ) - if not self.request.user.has_perm(perm, perm_obj): + 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 @@ -275,7 +279,7 @@ def retrieve(self, request, *args, **kwargs): 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 @@ -311,7 +315,7 @@ 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 = ObjectTagPermissionItem( @@ -319,7 +323,11 @@ def update(self, request, *args, **kwargs): 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." ) From 13feb1cedfc8851ff8fde8c7b41b59cf63da8d3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Sat, 14 Oct 2023 11:13:41 -0300 Subject: [PATCH 07/10] chore: bump version to `0.2.5` --- openedx_learning/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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" From 6b9198029ce64be1e1f4af2b106226867b35209d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 16 Oct 2023 15:55:13 -0300 Subject: [PATCH 08/10] fix: add can_view_object_tag_taxonomy --- openedx_tagging/core/tagging/rules.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/openedx_tagging/core/tagging/rules.py b/openedx_tagging/core/tagging/rules.py index b10d2f92..33e473ed 100644 --- a/openedx_tagging/core/tagging/rules.py +++ b/openedx_tagging/core/tagging/rules.py @@ -65,6 +65,15 @@ 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. + """ + return taxonomy.cast().enabled and can_view_taxonomy(user, taxonomy) + + @rules.predicate def can_view_object_tag_objectid(_user: UserType, _object_id: str) -> bool: """ @@ -164,6 +173,6 @@ def can_change_object_tag( # Users can tag objects using tags from any taxonomy that they have permission to view rules.add_perm("oel_tagging.view_objecttag_objectid", can_view_object_tag_objectid) -rules.add_perm("oel_tagging.view_objecttag_taxonomy", can_view_taxonomy) -rules.add_perm("oel_tagging.change_objecttag_taxonomy", can_view_taxonomy) +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) From eb599cee75d692a465c0085fd52211bed29095b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 16 Oct 2023 16:16:19 -0300 Subject: [PATCH 09/10] test: fix rules and docs --- .../core/tagging/test_rules.py | 2 +- .../core/tagging/test_views.py | 41 +++++++++---------- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/tests/openedx_tagging/core/tagging/test_rules.py b/tests/openedx_tagging/core/tagging/test_rules.py index 66af076f..f30cbd27 100644 --- a/tests/openedx_tagging/core/tagging/test_rules.py +++ b/tests/openedx_tagging/core/tagging/test_rules.py @@ -215,7 +215,7 @@ def test_object_tag_disabled_taxonomy(self, perm): 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( diff --git a/tests/openedx_tagging/core/tagging/test_views.py b/tests/openedx_tagging/core/tagging/test_views.py index 1291fac9..d97296ba 100644 --- a/tests/openedx_tagging/core/tagging/test_views.py +++ b/tests/openedx_tagging/core/tagging/test_views.py @@ -714,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): @@ -756,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), @@ -766,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): @@ -786,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): @@ -819,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): From bc8efc0550077972bd311aa9f7d3caec53e724a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Mon, 16 Oct 2023 16:16:34 -0300 Subject: [PATCH 10/10] docs: clarify difference from can_view_taxonomy --- openedx_tagging/core/tagging/rules.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/openedx_tagging/core/tagging/rules.py b/openedx_tagging/core/tagging/rules.py index 33e473ed..8249628f 100644 --- a/openedx_tagging/core/tagging/rules.py +++ b/openedx_tagging/core/tagging/rules.py @@ -70,7 +70,12 @@ 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)