From e9a70f456ca229a49331239d0e390c1b8faaad05 Mon Sep 17 00:00:00 2001
From: Geert Kapteijns <ghkapteijns@gmail.com>
Date: Sat, 25 Feb 2017 20:42:09 +0100
Subject: [PATCH] Extract submission comment logic to #new_comment
Fix bug that allows a forged post request to create a comment,
while the submission is not open for commenting.
---
comments/templates/comments/new_comment.html | 2 +-
comments/test_views.py | 55 ++++++++++++++++++-
comments/views.py | 19 +++++--
submissions/factories.py | 2 +-
.../submissions/submission_detail.html | 33 +----------
submissions/test_views.py | 18 ++++++
submissions/views.py | 37 +------------
theses/templates/theses/thesis_detail.html | 2 +-
8 files changed, 91 insertions(+), 77 deletions(-)
diff --git a/comments/templates/comments/new_comment.html b/comments/templates/comments/new_comment.html
index 572b9fd43..80f19afad 100644
--- a/comments/templates/comments/new_comment.html
+++ b/comments/templates/comments/new_comment.html
@@ -1,4 +1,4 @@
-{% if user.is_authenticated and thesislink.open_for_commenting and perms.scipost.can_submit_comments %}
+{% if user.is_authenticated and open_for_commenting and perms.scipost.can_submit_comments %}
<hr />
<div class="row">
diff --git a/comments/test_views.py b/comments/test_views.py
index 0f382843b..967c63d3b 100644
--- a/comments/test_views.py
+++ b/comments/test_views.py
@@ -1,9 +1,11 @@
from django.test import TestCase, RequestFactory, Client
from django.urls import reverse, reverse_lazy
from django.contrib.auth.models import Group
+from django.core.exceptions import PermissionDenied
from scipost.factories import ContributorFactory
from theses.factories import ThesisLinkFactory
+from submissions.factories import EICassignedSubmissionFactory
from .factories import CommentFactory
from .forms import CommentForm
@@ -22,7 +24,7 @@ class TestNewComment(TestCase):
contributor = ContributorFactory()
thesislink = ThesisLinkFactory()
valid_comment_data = model_form_data(CommentFactory.build(), CommentForm)
- target = reverse('theses:thesis', kwargs={'thesislink_id': thesislink.id})
+ target = reverse('comments:new_comment', kwargs={'object_id': thesislink.id, 'type_of_object': 'thesislink'})
comment_count = Comment.objects.filter(author=contributor).count()
self.assertEqual(comment_count, 0)
@@ -35,4 +37,53 @@ class TestNewComment(TestCase):
self.assertEqual(comment_count, 1)
response.client = Client()
- self.assertRedirects(response, reverse('theses:thesis', kwargs={"thesislink_id":thesislink.id}))
+ expected_redirect_link = reverse('theses:thesis', kwargs={'thesislink_id': thesislink.id})
+ self.assertRedirects(response, expected_redirect_link)
+
+ def test_submitting_comment_on_submission_creates_comment_and_redirects(self):
+ contributor = ContributorFactory()
+ submission = EICassignedSubmissionFactory()
+ submission.open_for_commenting = True
+ submission.save()
+ valid_comment_data = model_form_data(CommentFactory.build(), CommentForm)
+ target = reverse(
+ 'comments:new_comment',
+ kwargs={'object_id': submission.id, 'type_of_object': 'submission'},
+ )
+
+ comment_count = Comment.objects.filter(author=contributor).count()
+ self.assertEqual(comment_count, 0)
+
+ request = RequestFactory().post(target, valid_comment_data)
+ request.user = contributor.user
+ response = new_comment(request, object_id=submission.id, type_of_object='submission')
+
+ comment_count = Comment.objects.filter(author=contributor).count()
+ self.assertEqual(comment_count, 1)
+
+ response.client = Client()
+ expected_redirect_link = reverse(
+ 'submissions:submission',
+ kwargs={'arxiv_identifier_w_vn_nr': submission.arxiv_identifier_w_vn_nr}
+ )
+ self.assertRedirects(response, expected_redirect_link)
+
+
+ def test_submitting_comment_on_submission_that_is_not_open_for_commenting_should_be_impossible(self):
+ contributor = ContributorFactory()
+ submission = EICassignedSubmissionFactory()
+ submission.open_for_commenting = False
+ submission.save()
+ valid_comment_data = model_form_data(CommentFactory.build(), CommentForm)
+ target = reverse(
+ 'comments:new_comment',
+ kwargs={'object_id': submission.id, 'type_of_object': 'submission'},
+ )
+
+ comment_count = Comment.objects.filter(author=contributor).count()
+ self.assertEqual(comment_count, 0)
+
+ request = RequestFactory().post(target, valid_comment_data)
+ request.user = contributor.user
+ with self.assertRaises(PermissionDenied):
+ response = new_comment(request, object_id=submission.id, type_of_object='submission')
diff --git a/comments/views.py b/comments/views.py
index f07ed6d76..4b32e734f 100644
--- a/comments/views.py
+++ b/comments/views.py
@@ -3,6 +3,7 @@ from django.shortcuts import get_object_or_404, render, redirect
from django.contrib.auth.decorators import permission_required
from django.core.mail import EmailMessage
from django.core.urlresolvers import reverse
+from django.core.exceptions import PermissionDenied
from django.http import HttpResponseRedirect, Http404
from .models import Comment
@@ -11,7 +12,7 @@ from .forms import CommentForm, VetCommentForm, comment_refusal_dict
from scipost.models import Contributor, title_dict
from theses.models import ThesisLink
from submissions.utils import SubmissionUtils
-from submissions.models import Report
+from submissions.models import Submission, Report
@permission_required('scipost.can_submit_comments', raise_exception=True)
def new_comment(request, **kwargs):
@@ -22,7 +23,6 @@ def new_comment(request, **kwargs):
object_id = int(kwargs["object_id"])
type_of_object = kwargs["type_of_object"]
new_comment = Comment(
- # thesislink=thesislink,
author=author,
is_rem=form.cleaned_data['is_rem'],
is_que=form.cleaned_data['is_que'],
@@ -39,9 +39,17 @@ def new_comment(request, **kwargs):
if type_of_object == "thesislink":
thesislink = ThesisLink.objects.get(id=object_id)
new_comment.thesislink = thesislink
+ redirect_link = reverse('theses:thesis', kwargs={"thesislink_id":thesislink.id})
elif type_of_object == "submission":
- # TODO
- 1 + 1
+ submission = Submission.objects.get(id=object_id)
+ if not submission.open_for_commenting:
+ raise PermissionDenied
+
+ new_comment.submission = submission
+ redirect_link = reverse(
+ 'submissions:submission',
+ kwargs={"arxiv_identifier_w_vn_nr": submission.arxiv_identifier_w_vn_nr}
+ )
elif type_of_object == "commentary":
# TODO
1 + 1
@@ -49,8 +57,7 @@ def new_comment(request, **kwargs):
new_comment.save()
author.nr_comments = Comment.objects.filter(author=author).count()
author.save()
- if type_of_object == "thesislink":
- return redirect('theses:thesis', thesislink_id=object_id)
+ return redirect(redirect_link)
else:
# This view is only accessible by POST request
raise Http404
diff --git a/submissions/factories.py b/submissions/factories.py
index 7f07c3052..ade2eba39 100644
--- a/submissions/factories.py
+++ b/submissions/factories.py
@@ -25,7 +25,7 @@ class SubmissionFactory(factory.django.DjangoModelFactory):
class EICassignedSubmissionFactory(SubmissionFactory):
status = 'EICassigned'
editor_in_charge = factory.SubFactory(ContributorFactory)
-
+ open_for_commenting = True
def random_arxiv_identifier_with_version_number():
diff --git a/submissions/templates/submissions/submission_detail.html b/submissions/templates/submissions/submission_detail.html
index 68750602f..3e793e6ce 100644
--- a/submissions/templates/submissions/submission_detail.html
+++ b/submissions/templates/submissions/submission_detail.html
@@ -205,7 +205,7 @@
<p>
<a target="_blank" href="{{ reply.file_attachment.url }}">
{% if reply.file_attachment|is_image %}
- <img class="attachment attachment-comment" src="{{ reply.file_attachment.url }}">
+ <img class="attachment attachment-comment" src="{{ reply.file_attachment.url }}">
{% else %}
{{ reply.file_attachment|filename }}<br><small>{{ reply.file_attachment.size|filesizeformat }}</small>
{% endif %}
@@ -304,37 +304,8 @@
{% endif %}
-
-
{% include 'scipost/comments_block.html' %}
-{% if user.is_authenticated and submission.open_for_commenting and perms.scipost.can_submit_comments %}
-<hr>
-<div id="contribute_comment">
- <div class="row">
- <div class="col-12">
- <div class="panel">
- <h2>Contribute a Comment:</h2>
- </div>
- </div>
- </div>
- <div class="row">
- <div class="col-12">
- <form enctype="multipart/form-data" action="{% url 'submissions:submission' arxiv_identifier_w_vn_nr=submission.arxiv_identifier_w_vn_nr %}" method="post">
- {% csrf_token %}
- {% load crispy_forms_tags %}
- {% crispy form %}
- </form>
- </div>
- </div>
-
- <div class="row">
- <div class="col-12">
- <h3>Preview of your comment:</h3>
- <p id="preview-comment_text"></p>
- </div>
- </div>
-</div>
-{% endif %}
+{% include 'comments/new_comment.html' with object_id=submission.id type_of_object='submission' open_for_commenting=submission.open_for_commenting %}
{% endblock content %}
diff --git a/submissions/test_views.py b/submissions/test_views.py
index 5eef315b0..8cfec3410 100644
--- a/submissions/test_views.py
+++ b/submissions/test_views.py
@@ -3,6 +3,8 @@ from django.test import Client
from submissions.views import *
import django.core.urlresolvers
+from .factories import EICassignedSubmissionFactory
+
class PrefillUsingIdentifierTest(TestCase):
fixtures = ['permissions', 'groups', 'contributors']
@@ -45,3 +47,19 @@ class SubmitManuscriptTest(TestCase):
{**params, **extras})
self.assertEqual(response.status_code, 200)
+
+
+class SubmissionDetailTest(TestCase):
+ fixtures = ['permissions', 'groups', 'contributors']
+
+ def setUp(self):
+ self.client = Client()
+ self.submission = EICassignedSubmissionFactory()
+ self.target = reverse(
+ 'submissions:submission',
+ kwargs={'arxiv_identifier_w_vn_nr': self.submission.arxiv_identifier_w_vn_nr}
+ )
+
+ def test_status_code_200(self):
+ response = self.client.get(self.target)
+ self.assertEqual(response.status_code, 200)
diff --git a/submissions/views.py b/submissions/views.py
index f69b23d8c..2681da5f9 100644
--- a/submissions/views.py
+++ b/submissions/views.py
@@ -285,41 +285,8 @@ def submission_detail(request, arxiv_identifier_w_vn_nr):
other_versions = Submission.objects.filter(
arxiv_identifier_wo_vn_nr=submission.arxiv_identifier_wo_vn_nr
).exclude(pk=submission.id)
- if request.method == 'POST':
- form = CommentForm(request.POST, request.FILES)
- if form.is_valid():
- author = Contributor.objects.get(user=request.user)
- newcomment = Comment(
- submission=submission,
- author=author,
- is_rem=form.cleaned_data['is_rem'],
- is_que=form.cleaned_data['is_que'],
- is_ans=form.cleaned_data['is_ans'],
- is_obj=form.cleaned_data['is_obj'],
- is_rep=form.cleaned_data['is_rep'],
- is_val=form.cleaned_data['is_val'],
- is_lit=form.cleaned_data['is_lit'],
- is_sug=form.cleaned_data['is_sug'],
- file_attachment=form.cleaned_data['file_attachment'],
- comment_text=form.cleaned_data['comment_text'],
- remarks_for_editors=form.cleaned_data['remarks_for_editors'],
- date_submitted=timezone.now(),
- )
- newcomment.save()
- author.nr_comments = Comment.objects.filter(author=author).count()
- author.save()
- context = {'ack_header': 'Thank you for contributing a Comment.',
- 'ack_message': 'It will soon be vetted by an Editor.',
- 'followup_message': 'Back to the ',
- 'followup_link': reverse(
- 'submissions:submission',
- kwargs={'arxiv_identifier_w_vn_nr': newcomment.submission.arxiv_identifier_w_vn_nr}
- ),
- 'followup_link_label': ' Submission page you came from'
- }
- return render(request, 'scipost/acknowledgement.html', context)
- else:
- form = CommentForm()
+
+ form = CommentForm()
reports = submission.report_set.all()
try:
diff --git a/theses/templates/theses/thesis_detail.html b/theses/templates/theses/thesis_detail.html
index 8bca24d3e..edde03144 100644
--- a/theses/templates/theses/thesis_detail.html
+++ b/theses/templates/theses/thesis_detail.html
@@ -50,6 +50,6 @@
{% include 'scipost/comments_block.html' %}
-{% include 'comments/new_comment.html' with object_id=thesislink.id type_of_object='thesislink' %}
+{% include 'comments/new_comment.html' with object_id=thesislink.id type_of_object='thesislink' open_for_commenting=thesislink.open_for_commenting %}
{% endblock content %}
--
GitLab