diff --git a/dojo/authorization/api_permissions.py b/dojo/authorization/api_permissions.py index 5efa34837be..342a362c3da 100644 --- a/dojo/authorization/api_permissions.py +++ b/dojo/authorization/api_permissions.py @@ -16,6 +16,7 @@ user_has_permission, user_is_superuser_or_global_owner, ) +from dojo.authorization.roles_permissions import Permissions from dojo.importers.auto_create_context import AutoCreateContextManager from dojo.location.models import Location from dojo.models import ( @@ -401,6 +402,15 @@ class UserHasEngagementRelatedObjectPermission(BaseRelatedObjectPermission): } +class UserHasEngagementFilePermission(BaseRelatedObjectPermission): + permission_map = { + "get_permission": Permissions.Product_Tracking_Files_View, + "put_permission": Permissions.Product_Tracking_Files_Edit, + "delete_permission": Permissions.Product_Tracking_Files_Delete, + "post_permission": Permissions.Product_Tracking_Files_Add, + } + + class UserHasEngagementNotePermission(BaseRelatedObjectPermission): permission_map = { "get_permission": "view", @@ -462,6 +472,15 @@ class UserHasFindingRelatedObjectPermission(BaseRelatedObjectPermission): } +class UserHasFindingFilePermission(BaseRelatedObjectPermission): + permission_map = { + "get_permission": Permissions.Product_Tracking_Files_View, + "put_permission": Permissions.Product_Tracking_Files_Edit, + "delete_permission": Permissions.Product_Tracking_Files_Delete, + "post_permission": Permissions.Product_Tracking_Files_Add, + } + + class UserHasFindingNotePermission(BaseRelatedObjectPermission): permission_map = { "get_permission": "view", @@ -778,6 +797,15 @@ class UserHasTestRelatedObjectPermission(BaseRelatedObjectPermission): } +class UserHasTestFilePermission(BaseRelatedObjectPermission): + permission_map = { + "get_permission": Permissions.Product_Tracking_Files_View, + "put_permission": Permissions.Product_Tracking_Files_Edit, + "delete_permission": Permissions.Product_Tracking_Files_Delete, + "post_permission": Permissions.Product_Tracking_Files_Add, + } + + class UserHasTestNotePermission(BaseRelatedObjectPermission): permission_map = { "get_permission": "view", diff --git a/dojo/engagement/api/views.py b/dojo/engagement/api/views.py index 34ed278a358..76d6b90d5ce 100644 --- a/dojo/engagement/api/views.py +++ b/dojo/engagement/api/views.py @@ -219,7 +219,7 @@ def notes(self, request, pk=None): responses={status.HTTP_201_CREATED: api_v2_serializers.FileSerializer}, ) @action( - detail=True, methods=["get", "post"], parser_classes=(MultiPartParser,), permission_classes=[IsAuthenticated, permissions.UserHasEngagementRelatedObjectPermission], + detail=True, methods=["get", "post"], parser_classes=(MultiPartParser,), permission_classes=[IsAuthenticated, permissions.UserHasEngagementFilePermission], ) def files(self, request, pk=None): engagement = self.get_object() diff --git a/dojo/finding/api/views.py b/dojo/finding/api/views.py index 2df4cf00ad3..85a7a3db19b 100644 --- a/dojo/finding/api/views.py +++ b/dojo/finding/api/views.py @@ -459,7 +459,7 @@ def notes(self, request, pk=None): responses={status.HTTP_201_CREATED: api_v2_serializers.FileSerializer}, ) @action( - detail=True, methods=["get", "post"], parser_classes=(MultiPartParser,), permission_classes=(IsAuthenticated, permissions.UserHasFindingRelatedObjectPermission), + detail=True, methods=["get", "post"], parser_classes=(MultiPartParser,), permission_classes=(IsAuthenticated, permissions.UserHasFindingFilePermission), ) def files(self, request, pk=None): finding = self.get_object() @@ -497,7 +497,7 @@ def files(self, request, pk=None): @action( detail=True, methods=["get"], - url_path=r"files/download/(?P\d+)", permission_classes=(IsAuthenticated, permissions.UserHasFindingRelatedObjectPermission), + url_path=r"files/download/(?P\d+)", permission_classes=(IsAuthenticated, permissions.UserHasFindingFilePermission), ) def download_file(self, request, file_id, pk=None): finding = self.get_object() diff --git a/dojo/forms.py b/dojo/forms.py index c9075e07b6a..ccef242cbf1 100644 --- a/dojo/forms.py +++ b/dojo/forms.py @@ -572,6 +572,7 @@ def clean(self): ManageFileFormSet = modelformset_factory(FileUpload, extra=3, max_num=10, fields=["title", "file"], can_delete=True, formset=BaseManageFileFormSet) +AddOnlyManageFileFormSet = modelformset_factory(FileUpload, extra=3, max_num=10, fields=["title", "file"], can_delete=False, formset=BaseManageFileFormSet) # Risk acceptance forms live in dojo/risk_acceptance/ui/forms.py. Re-exported here for diff --git a/dojo/test/api/views.py b/dojo/test/api/views.py index 7c2066374c4..2a0480b2183 100644 --- a/dojo/test/api/views.py +++ b/dojo/test/api/views.py @@ -194,7 +194,7 @@ def notes(self, request, pk=None): responses={status.HTTP_201_CREATED: api_v2_serializers.FileSerializer}, ) @action( - detail=True, methods=["get", "post"], parser_classes=(MultiPartParser,), permission_classes=(IsAuthenticated, permissions.UserHasTestRelatedObjectPermission), + detail=True, methods=["get", "post"], parser_classes=(MultiPartParser,), permission_classes=(IsAuthenticated, permissions.UserHasTestFilePermission), ) def files(self, request, pk=None): test = self.get_object() @@ -233,7 +233,7 @@ def files(self, request, pk=None): detail=True, methods=["get"], url_path=r"files/download/(?P\d+)", - permission_classes=(IsAuthenticated, permissions.UserHasTestRelatedObjectPermission), + permission_classes=(IsAuthenticated, permissions.UserHasTestFilePermission), ) def download_file(self, request, file_id, pk=None): test = self.get_object() diff --git a/dojo/views.py b/dojo/views.py index 5c599511e05..4dad43aa117 100644 --- a/dojo/views.py +++ b/dojo/views.py @@ -10,8 +10,9 @@ from django.shortcuts import get_object_or_404, render from django.urls import reverse -from dojo.authorization.authorization import user_has_permission_or_403 -from dojo.forms import ManageFileFormSet +from dojo.authorization.authorization import user_has_permission, user_has_permission_or_403 +from dojo.authorization.roles_permissions import Permissions +from dojo.forms import AddOnlyManageFileFormSet, ManageFileFormSet from dojo.models import ( Engagement, FileUpload, @@ -42,34 +43,39 @@ def custom_bad_request_view(request, exception=None): def manage_files(request, oid, obj_type): if obj_type == "Engagement": obj = get_object_or_404(Engagement, pk=oid) - user_has_permission_or_403(request.user, obj, "edit") obj_vars = ("view_engagement", "engagement_set") elif obj_type == "Test": obj = get_object_or_404(Test, pk=oid) - user_has_permission_or_403(request.user, obj, "edit") obj_vars = ("view_test", "test_set") elif obj_type == "Finding": obj = get_object_or_404(Finding, pk=oid) - user_has_permission_or_403(request.user, obj, "edit") obj_vars = ("view_finding", "finding_set") else: raise Http404 - files_formset = ManageFileFormSet(queryset=obj.files.all()) + has_file_add_permission = user_has_permission(request.user, obj, Permissions.Product_Tracking_Files_Add) + has_file_edit_permission = user_has_permission(request.user, obj, Permissions.Product_Tracking_Files_Edit) + if not (has_file_add_permission or has_file_edit_permission): + raise PermissionDenied + + formset_class = ManageFileFormSet if has_file_edit_permission else AddOnlyManageFileFormSet + files_queryset = obj.files.all() if has_file_edit_permission else FileUpload.objects.none() + files_formset = formset_class(queryset=files_queryset) error = False if request.method == "POST": - files_formset = ManageFileFormSet( - request.POST, request.FILES, queryset=obj.files.all()) + files_formset = formset_class( + request.POST, request.FILES, queryset=files_queryset) if files_formset.is_valid(): # remove all from database and disk files_formset.save() - for o in files_formset.deleted_objects: - logger.debug("removing file: %s", o.file.name) - with suppress(FileNotFoundError): - (Path(settings.MEDIA_ROOT) / o.file.name).unlink() + for o in getattr(files_formset, "deleted_objects", []): + if has_file_edit_permission: + logger.debug("removing file: %s", o.file.name) + with suppress(FileNotFoundError): + (Path(settings.MEDIA_ROOT) / o.file.name).unlink() for o in files_formset.new_objects: logger.debug("adding file: %s", o.file.name) diff --git a/unittests/test_permissions_audit.py b/unittests/test_permissions_audit.py index fe7a31c24eb..4b982c1ccdc 100644 --- a/unittests/test_permissions_audit.py +++ b/unittests/test_permissions_audit.py @@ -1406,6 +1406,9 @@ def _client_for_user(self, user): client.credentials(HTTP_AUTHORIZATION="Token " + token.key) return client + def _authorize_reader_user(self): + self.product.authorized_users.add(self.reader_user) + def setUp(self): super().setUp() # Legacy auth collapses Reader/Writer/Maintainer/Owner into @@ -1493,11 +1496,13 @@ def test_engagement_notes_post_writer_allowed(self): # ── Engagement: files ────────────────────────────────────────────── - def test_engagement_files_post_reader_denied(self): + def test_engagement_files_post_member_allowed(self): + self._authorize_reader_user() client = self._client_for_user(self.reader_user) url = reverse("engagement-files", args=(self.engagement.id,)) - response = client.post(url, data={}, format="json") - self.assertEqual(response.status_code, 403, response.content) + test_file = SimpleUploadedFile("reader-proof.txt", b"engagement file content", content_type="text/plain") + response = client.post(url, data={"title": "reader proof", "file": test_file}, format="multipart") + self.assertEqual(response.status_code, 201, response.content) def test_engagement_files_post_writer_allowed(self): client = self._client_for_user(self.writer_user) @@ -1587,11 +1592,13 @@ def test_finding_notes_post_writer_allowed(self): # ── Finding: files ───────────────────────────────────────────────── - def test_finding_files_post_reader_denied(self): + def test_finding_files_post_member_allowed(self): + self._authorize_reader_user() client = self._client_for_user(self.reader_user) url = reverse("finding-files", args=(self.finding.id,)) - response = client.post(url, data={}, format="json") - self.assertEqual(response.status_code, 403, response.content) + test_file = SimpleUploadedFile("reader-evidence.txt", b"finding file content", content_type="text/plain") + response = client.post(url, data={"title": "reader evidence", "file": test_file}, format="multipart") + self.assertEqual(response.status_code, 201, response.content) def test_finding_files_post_writer_allowed(self): client = self._client_for_user(self.writer_user) @@ -1600,6 +1607,26 @@ def test_finding_files_post_writer_allowed(self): response = client.post(url, data={"title": "test evidence", "file": test_file}, format="multipart") self.assertEqual(response.status_code, 201, response.content) + def test_manage_files_member_can_upload_to_finding(self): + self._authorize_reader_user() + client = Client() + client.login(username="relobjperm_reader", password="testTEST1234!@#$") # noqa: S106 + test_file = SimpleUploadedFile("reader-ui-evidence.txt", b"finding file content", content_type="text/plain") + response = client.post( + reverse("manage_files", args=(self.finding.id, "Finding")), + data={ + "form-TOTAL_FORMS": "3", + "form-INITIAL_FORMS": "0", + "form-MIN_NUM_FORMS": "0", + "form-MAX_NUM_FORMS": "10", + "form-0-title": "reader ui evidence", + "form-0-file": test_file, + }, + ) + + self.assertEqual(response.status_code, 302, response.content) + self.assertTrue(self.finding.files.filter(title="reader ui evidence").exists()) + # ── Finding: remove_note (NotePermission — PATCH uses Edit) ──────── def test_finding_remove_note_reader_denied(self): @@ -1691,11 +1718,13 @@ def test_test_notes_post_writer_allowed(self): # ── Test: files ──────────────────────────────────────────────────── - def test_test_files_post_reader_denied(self): + def test_test_files_post_member_allowed(self): + self._authorize_reader_user() client = self._client_for_user(self.reader_user) url = reverse("test-files", args=(self.test.id,)) - response = client.post(url, data={}, format="json") - self.assertEqual(response.status_code, 403, response.content) + test_file = SimpleUploadedFile("reader-results.txt", b"test file content", content_type="text/plain") + response = client.post(url, data={"title": "reader results", "file": test_file}, format="multipart") + self.assertEqual(response.status_code, 201, response.content) def test_test_files_post_writer_allowed(self): client = self._client_for_user(self.writer_user)