From 3dd90f0b14eb12a7994adf49b374895849b143e8 Mon Sep 17 00:00:00 2001 From: svader0 Date: Mon, 17 Aug 2026 13:26:49 -0500 Subject: [PATCH] Require the change permission to edit a questionnaire's question set The question-set editor accepted either the questionnaire add permission or the change permission, so a role holding only the create permission could rewrite the question set of any existing questionnaire and reset the completion state of every answered copy of it. The three sibling questionnaire routes each require one permission, and the user permission chart documents editing an existing questionnaire as the change permission. Creating a questionnaire no longer redirects into the question-set editor unless the caller also holds the change permission, so a create-only role lands on the questionnaire list instead of a denial page. --- dojo/survey/ui/views.py | 9 +- .../test_questionnaire_question_set_authz.py | 121 ++++++++++++++++++ 2 files changed, 125 insertions(+), 5 deletions(-) create mode 100644 unittests/test_questionnaire_question_set_authz.py diff --git a/dojo/survey/ui/views.py b/dojo/survey/ui/views.py index 40ec641d850..d492e41802b 100644 --- a/dojo/survey/ui/views.py +++ b/dojo/survey/ui/views.py @@ -15,6 +15,7 @@ from dojo.authorization.authorization import ( user_has_configuration_permission, + user_has_configuration_permission_or_403, user_has_permission, user_has_permission_or_403, ) @@ -349,7 +350,8 @@ def create_questionnaire(request): messages.SUCCESS, "Questionnaire successfully created, you may now add questions.", extra_tags="alert-success") - if "add_questions" in request.POST: + if "add_questions" in request.POST and user_has_configuration_permission( + request.user, "dojo.change_engagement_survey"): return HttpResponseRedirect(reverse("edit_questionnaire_questions", args=(survey.id,))) return HttpResponseRedirect(reverse("questionnaire")) messages.add_message( @@ -366,12 +368,9 @@ def create_questionnaire(request): }) -# complex permission check inside the function def edit_questionnaire_questions(request, sid): survey = get_object_or_404(Engagement_Survey, id=sid) - if not user_has_configuration_permission(request.user, "dojo.add_engagement_survey") and \ - not user_has_configuration_permission(request.user, "dojo.change_engagement_survey"): - raise PermissionDenied + user_has_configuration_permission_or_403(request.user, "dojo.change_engagement_survey") answered_surveys = Answered_Survey.objects.filter(survey=survey) reverted = False diff --git a/unittests/test_questionnaire_question_set_authz.py b/unittests/test_questionnaire_question_set_authz.py new file mode 100644 index 00000000000..31f47a63334 --- /dev/null +++ b/unittests/test_questionnaire_question_set_authz.py @@ -0,0 +1,121 @@ +""" +Regression tests for authorization on ``edit_questionnaire_questions`` +(``dojo/survey/ui/views.py``), the route that replaces the question set of an +existing questionnaire and resets every answered instance of it to uncompleted. + +The route used to accept ``dojo.add_engagement_survey`` OR +``dojo.change_engagement_survey``, so a user holding only the create permission +could rewrite any questionnaire in the instance. The sibling routes each carry +one verb and one permission, and the permission chart documents editing an +existing questionnaire as the change permission. + +These tests pin the contract: only the change permission reaches this route. +""" +from django.contrib.auth.models import Permission +from django.urls import reverse +from django.utils import timezone + +from dojo.models import ( + Answered_Survey, + Dojo_User, + Engagement, + Engagement_Survey, + Product, + Product_Type, + TextQuestion, +) +from unittests.dojo_test_case import DojoTestCase + + +def _config_permission(codename): + return Permission.objects.get(codename=codename, content_type__app_label="dojo") + + +class EditQuestionnaireQuestionsAuthorizationTests(DojoTestCase): + + @classmethod + def setUpTestData(cls): + cls.prod_type = Product_Type.objects.create(name="qset_authz_pt") + cls.victim_product = Product.objects.create( + name="qset_authz_victim", description="v", prod_type=cls.prod_type, + ) + cls.victim_engagement = Engagement.objects.create( + name="qset_authz_victim_eng", + product=cls.victim_product, + target_start=timezone.now().date(), + target_end=timezone.now().date(), + ) + + cls.original_question = TextQuestion.objects.create(text="qset_authz_original", order=1) + cls.substitute_question = TextQuestion.objects.create(text="qset_authz_substitute", order=2) + + cls.questionnaire = Engagement_Survey.objects.create(name="qset_authz_template", description="t") + cls.questionnaire.questions.add(cls.original_question) + + cls.answered = Answered_Survey.objects.create( + survey=cls.questionnaire, + engagement=cls.victim_engagement, + completed=True, + answered_on=timezone.now().date(), + ) + + # Holds only the create permission, and no membership on the victim tenant. + cls.creator = Dojo_User.objects.create(username="qset_authz_creator", is_active=True) + cls.creator.user_permissions.add(_config_permission("add_engagement_survey")) + + # Holds neither questionnaire permission. + cls.outsider = Dojo_User.objects.create(username="qset_authz_outsider", is_active=True) + + # Holds the change permission (positive control). + cls.editor = Dojo_User.objects.create(username="qset_authz_editor", is_active=True) + cls.editor.user_permissions.add(_config_permission("change_engagement_survey")) + + def _url(self): + return reverse("edit_questionnaire_questions", args=(self.questionnaire.id,)) + + def _assert_questionnaire_untouched(self): + self.answered.refresh_from_db() + self.assertEqual( + [self.original_question.id], + list(self.questionnaire.questions.values_list("id", flat=True)), + ) + self.assertTrue(self.answered.completed) + self.assertIsNotNone(self.answered.answered_on) + + def test_get_denied_for_create_only_user(self): + self.client.force_login(self.creator) + response = self.client.get(self._url()) + self.assertEqual(response.status_code, 400) + + def test_post_denied_for_create_only_user(self): + self.client.force_login(self.creator) + response = self.client.post(self._url(), data={"questions": [self.substitute_question.id]}) + self.assertEqual(response.status_code, 400) + self._assert_questionnaire_untouched() + + def test_post_denied_for_user_without_questionnaire_permissions(self): + self.client.force_login(self.outsider) + response = self.client.post(self._url(), data={"questions": [self.substitute_question.id]}) + self.assertEqual(response.status_code, 400) + self._assert_questionnaire_untouched() + + def test_user_with_change_permission_can_edit_the_question_set(self): + self.client.force_login(self.editor) + self.assertEqual(self.client.get(self._url()).status_code, 200) + response = self.client.post(self._url(), data={"questions": [self.substitute_question.id]}) + self.assertEqual(response.status_code, 302) + self.answered.refresh_from_db() + self.assertEqual( + [self.substitute_question.id], + list(self.questionnaire.questions.values_list("id", flat=True)), + ) + self.assertFalse(self.answered.completed) + + def test_create_only_user_is_not_redirected_into_the_question_set_editor(self): + self.client.force_login(self.creator) + response = self.client.post( + reverse("create_questionnaire"), + data={"name": "qset_authz_new", "description": "d", "active": True, "add_questions": ""}, + ) + self.assertEqual(response.status_code, 302) + self.assertEqual(reverse("questionnaire"), response["Location"])