fix #1617 validate author can only unassociate their own files in do_revisions

src/core/files.py

@@ -515,17 +515,18 @@ def serve_pdf_galley_to_browser(request, file, article):
         raise Http404
 
 
-def delete_file(article_object, file_object):
-    """Deletes a file. Note: the actual file is not deleted, this just removes the association of the file with an
-    article.
+def unassociate_file(article_object, file_object):
+    """Removes the association between a file and an article. The file itself
+    is not deleted from disk and its history is retained; only the manuscript
+    or data/figure relationship is removed.
 
     :param article_object: the article associated with the file
-    :param file_object: the file object to delete
+    :param file_object: the file object to unassociate
     :return: None
     """
     if article_object.manuscript_files.filter(id=file_object.id).exists():
         article_object.manuscript_files.remove(file_object)
-    else:
+    elif article_object.data_figure_files.filter(id=file_object.id).exists():
         article_object.data_figure_files.remove(file_object)
 
 

src/review/logic.py

@@ -457,7 +457,15 @@ def get_draft_email_message(request, article):
     return render_template.get_message_content(request, email_context, "draft_message")
 
 
-def group_files(article, reviews):
+def group_files(article, reviews, manageable=False):
+    """Collects the files an author sees for a revision request.
+
+    :param article: the Article being revised
+    :param reviews: completed, author-consumable ReviewAssignments
+    :param manageable: when True, only files the author is allowed to manage
+        (manuscript and data/figure files) are returned. Review files are
+        excluded because the author may download but not disassociate them.
+    """
     files = list()
 
     for file in article.manuscript_files.all():
@@ -466,9 +474,10 @@ def group_files(article, reviews):
     for file in article.data_figure_files.all():
         files.append(file)
 
-    for review in reviews:
-        if review.for_author_consumption and review.display_review_file:
-            files.append(review.review_file)
+    if not manageable:
+        for review in reviews:
+            if review.for_author_consumption and review.display_review_file:
+                files.append(review.review_file)
 
     return files
 

src/review/tests/test_views.py

@@ -529,8 +529,8 @@ def test_shared_review_download_view(self):
             403,
         )
 
-        # finally, delete the file from disk
-        files.delete_file(article_with_completed_reviews, file)
+        # finally, unassociate the file from the article
+        files.unassociate_file(article_with_completed_reviews, file)
 
     def test_withdrawing_review_assignment(self):
         review_to_withdraw, created = (
@@ -1441,3 +1441,119 @@ def test_rolled_back_article_offers_decision_actions(self):
             reverse("decision_helper", kwargs={"article_id": self.article.pk}),
         )
         self.assertNotContains(response, "Move to Next Stage")
+
+
+class DoRevisionsFileValidationTests(TestCase):
+    """Regression tests for #1617.
+
+    An author completing a revision request may disassociate their own
+    manuscript and data/figure files, but must not be able to disassociate
+    any other file (e.g. a review file) by tampering with the posted file id.
+    """
+
+    @classmethod
+    def setUpTestData(cls):
+        cls.press = helpers.create_press()
+        cls.journal_one, cls.journal_two = helpers.create_journals()
+        cls.editor = helpers.create_editor(cls.journal_one)
+        cls.author = helpers.create_author(cls.journal_one)
+        cls.other_author = helpers.create_author(
+            cls.journal_one,
+            email="otherauthor@example.com",
+        )
+        cls.article = helpers.create_article(
+            cls.journal_one,
+            stage=submission_models.STAGE_UNDER_REVIEW,
+            owner=cls.author,
+            correspondence_author=cls.author,
+        )
+        cls.author.snapshot_as_author(cls.article)
+
+        cls.manuscript_file = core_models.File.objects.create(
+            mime_type="text/plain",
+            original_filename="manuscript.txt",
+            uuid_filename="manuscript.txt",
+            label="Manuscript",
+            owner=cls.author,
+            is_galley=False,
+            privacy="owner",
+        )
+        cls.article.manuscript_files.add(cls.manuscript_file)
+
+        cls.review_round = review_models.ReviewRound.objects.create(
+            article=cls.article,
+            round_number=1,
+        )
+        cls.review_file = core_models.File.objects.create(
+            mime_type="text/plain",
+            original_filename="review.txt",
+            uuid_filename="review.txt",
+            label="Review file",
+            owner=cls.editor,
+            is_galley=False,
+            privacy="owner",
+        )
+        cls.review_round.review_files.add(cls.review_file)
+        cls.review_assignment = helpers.create_review_assignment(
+            journal=cls.journal_one,
+            article=cls.article,
+            editor=cls.editor,
+            review_round=cls.review_round,
+            review_file=cls.review_file,
+            is_complete=True,
+        )
+        cls.review_assignment.for_author_consumption = True
+        cls.review_assignment.display_review_file = True
+        cls.review_assignment.save()
+
+        cls.revision_request = helpers.create_revision_request(
+            cls.article,
+            cls.editor,
+        )
+
+    def do_revisions_url(self):
+        return reverse(
+            "do_revisions",
+            kwargs={
+                "article_id": self.article.pk,
+                "revision_id": self.revision_request.pk,
+            },
+        )
+
+    def test_author_can_delete_own_manuscript_file(self):
+        """The author may still disassociate a manuscript file of the article."""
+        self.client.force_login(self.author)
+        response = self.client.post(
+            self.do_revisions_url(),
+            {"delete": self.manuscript_file.pk},
+            SERVER_NAME=self.journal_one.domain,
+        )
+        self.assertEqual(response.status_code, 302)
+        self.assertFalse(
+            self.article.manuscript_files.filter(pk=self.manuscript_file.pk).exists()
+        )
+        self.assertTrue(
+            self.revision_request.actions.filter(text__contains="deleted").exists()
+        )
+
+    def test_author_cannot_delete_review_file(self):
+        """Tampering the file id with a review file must not disassociate it."""
+        self.client.force_login(self.author)
+        response = self.client.post(
+            self.do_revisions_url(),
+            {"delete": self.review_file.pk},
+            SERVER_NAME=self.journal_one.domain,
+            follow=True,
+        )
+        self.assertContains(
+            response,
+            "Given file ID not found in article files.",
+        )
+        # The review file remains associated with the review round.
+        self.assertTrue(
+            self.review_round.review_files.filter(pk=self.review_file.pk).exists()
+        )
+        # No deletion was logged against the revision request.
+        self.assertFalse(
+            self.revision_request.actions.filter(text__contains="deleted").exists()
+        )

src/review/views.py

@@ -2297,7 +2297,10 @@ def do_revisions(request, article_id, revision_id):
     ).exclude(decision="withdrawn")
 
     form = forms.DoRevisions(instance=revision_request)
-    revision_files = logic.group_files(revision_request.article, reviews)
+    manageable_files = logic.group_files(
+        revision_request.article, reviews, manageable=True
+    )
+    downloadable_files = logic.group_files(revision_request.article, reviews)
 
     if request.POST:
         post_redirect = reverse(
@@ -2307,12 +2310,19 @@ def do_revisions(request, article_id, revision_id):
         if "delete" in request.POST:
             file_id = request.POST.get("delete")
             file = get_object_or_404(core_models.File, pk=file_id)
-            files.delete_file(revision_request.article, file)
-            logic.log_revision_event(
-                "File {0} ({1}) deleted.".format(file.id, file.original_filename),
-                request.user,
-                revision_request,
-            )
+            if file in manageable_files:
+                files.unassociate_file(revision_request.article, file)
+                logic.log_revision_event(
+                    "File {0} ({1}) deleted.".format(file.id, file.original_filename),
+                    request.user,
+                    revision_request,
+                )
+            else:
+                messages.add_message(
+                    request,
+                    messages.WARNING,
+                    _("Given file ID not found in article files."),
+                )
             return redirect(post_redirect)
 
         elif "save" in request.POST:
@@ -2354,7 +2364,7 @@ def do_revisions(request, article_id, revision_id):
     if request.GET.get("file_id", None):
         file_id = request.GET.get("file_id")
         file = get_object_or_404(core_models.File, pk=file_id)
-        if file in revision_files:
+        if file in downloadable_files:
             logic.log_revision_event(
                 "Downloaded file {0} ({1}).".format(file.label, file.original_filename),
                 request.user,