diff --git a/docs/decisions/0009-tagging-administrators.rst b/docs/decisions/0009-tagging-administrators.rst index 652dcd24..33b0ab52 100644 --- a/docs/decisions/0009-tagging-administrators.rst +++ b/docs/decisions/0009-tagging-administrators.rst @@ -26,6 +26,8 @@ But because permissions #2 + #3 require access to the edx-platform CMS model `Co Per `OEP-9`_, ``openedx_tagging`` will allow applications to use the standard Django API to query permissions, for example: ``user.has_perm('openedx_tagging.edit_taxonomy', taxonomy)``, and the appropriate permissions will be applied in that application's context. +These rules will be enforced in the tagging `views`_, not the API or models, so that external code using this library need not have a logged-in user in order to call the API. So please use with care. + Rejected Alternatives --------------------- @@ -37,3 +39,4 @@ This is a standard way to grant access in Django apps, but it is not used in Ope .. _get_organizations: https://github.com/openedx/edx-platform/blob/4dc35c73ffa6d6a1dcb6e9ea1baa5bed40721125/cms/djangoapps/contentstore/views/course.py#L1958 .. _CourseCreator: https://github.com/openedx/edx-platform/blob/4dc35c73ffa6d6a1dcb6e9ea1baa5bed40721125/cms/djangoapps/course_creators/models.py#L27 .. _OEP-9: https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0009-bp-permissions.html +.. _views: https://github.com/dfunckt/django-rules#permissions-in-views diff --git a/openedx_tagging/core/tagging/rules.py b/openedx_tagging/core/tagging/rules.py new file mode 100644 index 00000000..7ef984b8 --- /dev/null +++ b/openedx_tagging/core/tagging/rules.py @@ -0,0 +1,74 @@ +"""Django rules-based permissions for tagging""" + +import rules + +# Global staff are taxonomy admins. +# (Superusers can already do anything) +is_taxonomy_admin = rules.is_staff + + +@rules.predicate +def can_view_taxonomy(user, taxonomy=None): + """ + Anyone can view an enabled taxonomy, + but only taxonomy admins can view a disabled taxonomy. + """ + return (taxonomy and taxonomy.enabled) or is_taxonomy_admin(user) + + +@rules.predicate +def can_change_taxonomy(user, taxonomy=None): + """ + Even taxonomy admins cannot change system taxonomies. + """ + return is_taxonomy_admin(user) and ( + not taxonomy or not taxonomy or (taxonomy and not taxonomy.system_defined) + ) + + +@rules.predicate +def can_change_taxonomy_tag(user, tag=None): + """ + Even taxonomy admins cannot add tags to system taxonomies (their tags are system-defined), or free-text taxonomies + (these don't have predefined tags). + """ + return is_taxonomy_admin(user) and ( + not tag + or not tag.taxonomy + or ( + tag.taxonomy + and not tag.taxonomy.allow_free_text + and not tag.taxonomy.system_defined + ) + ) + + +@rules.predicate +def can_change_object_tag(user, object_tag=None): + """ + Taxonomy admins can create or modify object tags on enabled taxonomies. + """ + return is_taxonomy_admin(user) and ( + not object_tag + or not object_tag.taxonomy + or (object_tag.taxonomy and object_tag.taxonomy.enabled) + ) + + +# Taxonomy +rules.add_perm("oel_tagging.add_taxonomy", can_change_taxonomy) +rules.add_perm("oel_tagging.change_taxonomy", can_change_taxonomy) +rules.add_perm("oel_tagging.delete_taxonomy", can_change_taxonomy) +rules.add_perm("oel_tagging.view_taxonomy", can_view_taxonomy) + +# Tag +rules.add_perm("oel_tagging.add_tag", can_change_taxonomy_tag) +rules.add_perm("oel_tagging.change_tag", can_change_taxonomy_tag) +rules.add_perm("oel_tagging.delete_tag", is_taxonomy_admin) +rules.add_perm("oel_tagging.view_tag", rules.always_allow) + +# ObjectTag +rules.add_perm("oel_tagging.add_object_tag", can_change_object_tag) +rules.add_perm("oel_tagging.change_object_tag", can_change_object_tag) +rules.add_perm("oel_tagging.delete_object_tag", is_taxonomy_admin) +rules.add_perm("oel_tagging.view_object_tag", rules.always_allow) diff --git a/projects/dev.py b/projects/dev.py index 99903c3f..a96473d4 100644 --- a/projects/dev.py +++ b/projects/dev.py @@ -41,6 +41,8 @@ # REST API "rest_framework", "openedx_learning.rest_api.apps.RESTAPIConfig", + # django-rules based authorization + 'rules.apps.AutodiscoverRulesConfig', # Tagging Core Apps "openedx_tagging.core.tagging.apps.TaggingConfig", @@ -48,6 +50,10 @@ "debug_toolbar", ) +AUTHENTICATION_BACKENDS = [ + 'rules.permissions.ObjectPermissionBackend', +] + MIDDLEWARE = [ "debug_toolbar.middleware.DebugToolbarMiddleware", "django.middleware.security.SecurityMiddleware", diff --git a/requirements/base.in b/requirements/base.in index aa00dddd..d17e0639 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -4,3 +4,5 @@ Django<5.0 # Web application framework djangorestframework<4.0 # REST API + +rules<4.0 # Django extension for rules-based authorization checks diff --git a/requirements/base.txt b/requirements/base.txt index 97ec0354..8c0f00b6 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -17,6 +17,8 @@ pytz==2023.3 # via # django # djangorestframework +rules==3.3 + # via -r requirements/base.in sqlparse==0.4.4 # via django typing-extensions==4.6.3 diff --git a/requirements/dev.txt b/requirements/dev.txt index e816277f..add9cf1d 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -169,6 +169,8 @@ mdurl==0.1.2 # via # -r requirements/quality.txt # markdown-it-py +mock==5.0.2 + # via -r requirements/quality.txt more-itertools==9.1.0 # via # -r requirements/quality.txt @@ -296,6 +298,8 @@ rich==13.4.2 # via # -r requirements/quality.txt # twine +rules==3.3 + # via -r requirements/quality.txt secretstorage==3.3.3 # via # -r requirements/quality.txt diff --git a/requirements/doc.txt b/requirements/doc.txt index b4e6b5d5..d9a4c3c2 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -83,6 +83,8 @@ markupsafe==2.1.3 # via # -r requirements/test.txt # jinja2 +mock==5.0.2 + # via -r requirements/test.txt mysqlclient==2.1.1 # via -r requirements/test.txt packaging==23.1 @@ -139,6 +141,8 @@ requests==2.31.0 # via sphinx restructuredtext-lint==1.4.0 # via doc8 +rules==3.3 + # via -r requirements/test.txt six==1.16.0 # via bleach snowballstemmer==2.2.0 diff --git a/requirements/quality.txt b/requirements/quality.txt index c70f66a7..c580bd7e 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -104,6 +104,8 @@ mccabe==0.7.0 # via pylint mdurl==0.1.2 # via markdown-it-py +mock==5.0.2 + # via -r requirements/test.txt more-itertools==9.1.0 # via jaraco-classes mysqlclient==2.1.1 @@ -182,6 +184,8 @@ rfc3986==2.0.0 # via twine rich==13.4.2 # via twine +rules==3.3 + # via -r requirements/test.txt secretstorage==3.3.3 # via keyring six==1.16.0 diff --git a/requirements/test.in b/requirements/test.in index 6b4e096d..1d521351 100644 --- a/requirements/test.in +++ b/requirements/test.in @@ -13,3 +13,4 @@ pytest-cov # pytest extension for code coverage statistics pytest-django # pytest extension for better Django support code-annotations # provides commands used by the pii_check make target. ddt # supports data driven tests +mock # supports overriding classes and methods in tests diff --git a/requirements/test.txt b/requirements/test.txt index dafcddb0..e64d8190 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -38,6 +38,8 @@ jinja2==3.1.2 # via code-annotations markupsafe==2.1.3 # via jinja2 +mock==5.0.2 + # via -r requirements/test.in mysqlclient==2.1.1 # via -r requirements/test.in packaging==23.1 @@ -64,6 +66,8 @@ pytz==2023.3 # djangorestframework pyyaml==6.0 # via code-annotations +rules==3.3 + # via -r requirements/base.txt sqlparse==0.4.4 # via # -r requirements/base.txt diff --git a/test_settings.py b/test_settings.py index 46e52fa5..2e41694e 100644 --- a/test_settings.py +++ b/test_settings.py @@ -35,6 +35,8 @@ def root(*args): # Admin # 'django.contrib.admin', # 'django.contrib.admindocs', + # django-rules based authorization + 'rules.apps.AutodiscoverRulesConfig', # Our own apps "openedx_learning.core.components.apps.ComponentsConfig", "openedx_learning.core.contents.apps.ContentsConfig", @@ -42,6 +44,10 @@ def root(*args): "openedx_tagging.core.tagging.apps.TaggingConfig", ] +AUTHENTICATION_BACKENDS = [ + 'rules.permissions.ObjectPermissionBackend', +] + LOCALE_PATHS = [ root("openedx_learning", "conf", "locale"), ] diff --git a/tests/openedx_tagging/core/tagging/test_rules.py b/tests/openedx_tagging/core/tagging/test_rules.py new file mode 100644 index 00000000..2c2b4e74 --- /dev/null +++ b/tests/openedx_tagging/core/tagging/test_rules.py @@ -0,0 +1,230 @@ +"""Tests tagging rules-based permissions""" + +import ddt +from django.contrib.auth import get_user_model +from django.test.testcases import TestCase +from mock import Mock + +from openedx_tagging.core.tagging.models import ObjectTag, Tag, Taxonomy + +from .test_models import TestTagTaxonomyMixin + +User = get_user_model() + + +@ddt.ddt +class TestRulesTagging(TestTagTaxonomyMixin, TestCase): + """ + Tests that the expected rules have been applied to the tagging models. + """ + + def setUp(self): + super().setUp() + self.superuser = User.objects.create( + username="superuser", + email="superuser@example.com", + is_superuser=True, + ) + self.staff = User.objects.create( + username="staff", + email="staff@example.com", + is_staff=True, + ) + self.learner = User.objects.create( + username="learner", + email="learner@example.com", + ) + + self.object_tag = ObjectTag( + taxonomy=self.taxonomy, + tag=self.bacteria, + ) + self.object_tag.resync() + self.object_tag.save() + + # Taxonomy + + @ddt.data( + "oel_tagging.add_taxonomy", + "oel_tagging.change_taxonomy", + ) + def test_add_change_taxonomy(self, perm): + """Taxonomy administrators can create or modify any Taxonomy""" + assert self.superuser.has_perm(perm) + assert self.superuser.has_perm(perm, self.taxonomy) + assert self.staff.has_perm(perm) + assert self.staff.has_perm(perm, self.taxonomy) + assert not self.learner.has_perm(perm) + assert not self.learner.has_perm(perm, self.taxonomy) + + @ddt.data( + "oel_tagging.add_taxonomy", + "oel_tagging.change_taxonomy", + "oel_tagging.delete_taxonomy", + ) + def test_system_taxonomy(self, perm): + """Taxonomy administrators cannot edit system taxonomies""" + # TODO: use SystemTaxonomy when available + system_taxonomy = Mock(spec=Taxonomy) + system_taxonomy.system_defined.return_value = True + assert self.superuser.has_perm(perm, system_taxonomy) + assert not self.staff.has_perm(perm, system_taxonomy) + assert not self.learner.has_perm(perm, system_taxonomy) + + @ddt.data( + True, + False, + ) + def test_delete_taxonomy(self, enabled): + """Taxonomy administrators can delete any Taxonomy""" + self.taxonomy.enabled = enabled + assert self.superuser.has_perm("oel_tagging.delete_taxonomy") + assert self.superuser.has_perm("oel_tagging.delete_taxonomy", self.taxonomy) + assert self.staff.has_perm("oel_tagging.delete_taxonomy") + assert self.staff.has_perm("oel_tagging.delete_taxonomy", self.taxonomy) + assert not self.learner.has_perm("oel_tagging.delete_taxonomy") + assert not self.learner.has_perm("oel_tagging.delete_taxonomy", self.taxonomy) + + @ddt.data( + True, + False, + ) + def test_view_taxonomy_enabled(self, enabled): + """Anyone can see enabled taxonomies, but learners cannot see disabled taxonomies""" + self.taxonomy.enabled = enabled + assert self.superuser.has_perm("oel_tagging.view_taxonomy") + assert self.superuser.has_perm("oel_tagging.view_taxonomy", self.taxonomy) + assert self.staff.has_perm("oel_tagging.view_taxonomy") + assert self.staff.has_perm("oel_tagging.view_taxonomy", self.taxonomy) + assert not self.learner.has_perm("oel_tagging.view_taxonomy") + assert ( + self.learner.has_perm("oel_tagging.view_taxonomy", self.taxonomy) == enabled + ) + + # Tag + + @ddt.data( + "oel_tagging.add_tag", + "oel_tagging.change_tag", + ) + def test_add_change_tag(self, perm): + """Taxonomy administrators can modify tags on non-free-text taxonomies""" + assert self.superuser.has_perm(perm) + assert self.superuser.has_perm(perm, self.bacteria) + assert self.staff.has_perm(perm) + assert self.staff.has_perm(perm, self.bacteria) + assert not self.learner.has_perm(perm) + assert not self.learner.has_perm(perm, self.bacteria) + + @ddt.data( + "oel_tagging.add_tag", + "oel_tagging.change_tag", + ) + def test_tag_free_text_taxonomy(self, perm): + """Taxonomy administrators cannot modify tags on a free-text Taxonomy""" + self.taxonomy.allow_free_text = True + self.taxonomy.save() + assert self.superuser.has_perm(perm, self.bacteria) + assert not self.staff.has_perm(perm, self.bacteria) + assert not self.learner.has_perm(perm, self.bacteria) + + @ddt.data( + "oel_tagging.add_tag", + "oel_tagging.change_tag", + "oel_tagging.delete_tag", + ) + def test_tag_no_taxonomy(self, perm): + """Taxonomy administrators can modify any Tag, even those with no Taxonnmy.""" + tag = Tag() + assert self.superuser.has_perm(perm, tag) + assert self.staff.has_perm(perm, tag) + assert not self.learner.has_perm(perm, tag) + + @ddt.data( + True, + False, + ) + def test_delete_tag(self, allow_free_text): + """Taxonomy administrators can delete any Tag, even those associated with a free-text Taxonomy.""" + self.taxonomy.allow_free_text = allow_free_text + self.taxonomy.save() + assert self.superuser.has_perm("oel_tagging.delete_tag") + assert self.superuser.has_perm("oel_tagging.delete_tag", self.bacteria) + assert self.staff.has_perm("oel_tagging.delete_tag") + assert self.staff.has_perm("oel_tagging.delete_tag", self.bacteria) + assert not self.learner.has_perm("oel_tagging.delete_tag") + assert not self.learner.has_perm("oel_tagging.delete_tag", self.bacteria) + + def test_view_tag(self): + """Anyone can view any Tag""" + assert self.superuser.has_perm("oel_tagging.view_tag") + assert self.superuser.has_perm("oel_tagging.view_tag", self.bacteria) + assert self.staff.has_perm("oel_tagging.view_tag") + assert self.staff.has_perm("oel_tagging.view_tag", self.bacteria) + assert self.learner.has_perm("oel_tagging.view_tag") + assert self.learner.has_perm("oel_tagging.view_tag", self.bacteria) + + # ObjectTag + + @ddt.data( + "oel_tagging.add_object_tag", + "oel_tagging.change_object_tag", + ) + def test_add_change_object_tag(self, perm): + """Taxonomy administrators can create/edit an ObjectTag with an enabled Taxonomy""" + assert self.superuser.has_perm(perm) + assert self.superuser.has_perm(perm, self.object_tag) + assert self.staff.has_perm(perm) + assert self.staff.has_perm(perm, self.object_tag) + assert not self.learner.has_perm(perm) + assert not self.learner.has_perm(perm, self.object_tag) + + @ddt.data( + "oel_tagging.add_object_tag", + "oel_tagging.change_object_tag", + ) + def test_object_tag_disabled_taxonomy(self, perm): + """Taxonomy administrators cannot create/edit an ObjectTag with a disabled Taxonomy""" + self.taxonomy.enabled = False + self.taxonomy.save() + assert self.superuser.has_perm(perm, self.object_tag) + assert not self.staff.has_perm(perm, self.object_tag) + assert not self.learner.has_perm(perm, self.object_tag) + + @ddt.data( + True, + False, + ) + def test_delete_object_tag(self, enabled): + """Taxonomy administrators can delete any ObjectTag, even those associated with a disabled Taxonomy.""" + self.taxonomy.enabled = enabled + self.taxonomy.save() + assert self.superuser.has_perm("oel_tagging.delete_object_tag") + assert self.superuser.has_perm("oel_tagging.delete_object_tag", self.object_tag) + assert self.staff.has_perm("oel_tagging.delete_object_tag") + assert self.staff.has_perm("oel_tagging.delete_object_tag", self.object_tag) + assert not self.learner.has_perm("oel_tagging.delete_object_tag") + assert not self.learner.has_perm( + "oel_tagging.delete_object_tag", self.object_tag + ) + + @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() + assert self.superuser.has_perm(perm, object_tag) + assert self.staff.has_perm(perm, object_tag) + assert not self.learner.has_perm(perm, object_tag) + + def test_view_object_tag(self): + """Anyone can view any ObjectTag""" + assert self.superuser.has_perm("oel_tagging.view_object_tag") + assert self.superuser.has_perm("oel_tagging.view_object_tag", self.object_tag) + assert self.staff.has_perm("oel_tagging.view_object_tag") + assert self.staff.has_perm("oel_tagging.view_object_tag", self.object_tag) + assert self.learner.has_perm("oel_tagging.view_object_tag") + assert self.learner.has_perm("oel_tagging.view_object_tag", self.object_tag)