diff --git a/src/core/middleware.py b/src/core/middleware.py
index fde5b9be51..061564aa5d 100755
--- a/src/core/middleware.py
+++ b/src/core/middleware.py
@@ -167,7 +167,9 @@ def process_request(request):
"general", "maintenance_message", request.journal
)
request.META["maintenance_mode"] = maintenance_mode_message
- raise PermissionDenied(request, maintenance_mode_message)
+ # The message is rendered as the 403 page's explanation, so
+ # pass it alone: adding the request made str(exception) a tuple.
+ raise PermissionDenied(maintenance_mode_message.value)
class CounterCookieMiddleware(BaseMiddleware):
diff --git a/src/security/const.py b/src/security/const.py
new file mode 100644
index 0000000000..b12b31d41c
--- /dev/null
+++ b/src/security/const.py
@@ -0,0 +1,45 @@
+from django.utils.translation import gettext_lazy as _
+
+from utils.const import EnumContains
+
+
+class AccessDeniedMessages(EnumContains):
+ """Explanations shown to a reader who has been denied access."""
+
+ SIGN_IN_REQUIRED = _("You need to sign in to view this page.")
+
+ # Shown when a page needed a role the account does not hold.
+ WRONG_ACCOUNT_ADVICE = _(
+ "If you have more than one account, sign out and sign in again with "
+ "the correct account."
+ )
+
+ # Shown when a task link was opened by the wrong account.
+ TASK_INVITATION_ADVICE = _(
+ "If you have more than one account, sign out and sign in again using "
+ "the email address that received the task invitation."
+ )
+
+ # These describe the account making the request and never the account the
+ # task belongs to. The same message answers a task that belongs to
+ # somebody else, one on another journal, and one that does not exist, so
+ # that the page cannot be used to discover which ids are real.
+ REVIEW_NOT_ASSIGNED = _("This review is not assigned to %(email)s.")
+ COPYEDIT_NOT_ASSIGNED = _("This copyediting task is not assigned to %(email)s.")
+ PROOFING_NOT_ASSIGNED = _("This proofing task is not assigned to %(email)s.")
+ TYPESETTING_NOT_ASSIGNED = _("This typesetting task is not assigned to %(email)s.")
+ ARTICLE_NOT_ASSOCIATED = _(
+ "This article is not associated with the account %(email)s."
+ )
+
+ REVIEW_LINK_INVALID = _(
+ "This review link is no longer valid. Ask the editor who invited you "
+ "to send a new invitation."
+ )
+ REVIEW_STAGE_PASSED = _(
+ "This article has moved past the review stage, so the review is now "
+ "closed. You do not need to do anything further. Contact the editor "
+ "if you were expecting to complete this review."
+ )
+ TASK_CANCELLED = _("This task was cancelled, so there is nothing left to do.")
+ TASK_COMPLETED = _("You have already completed this task.")
diff --git a/src/security/decorators.py b/src/security/decorators.py
index 36df526d21..f4ab985d65 100755
--- a/src/security/decorators.py
+++ b/src/security/decorators.py
@@ -19,6 +19,8 @@
from submission import models
from copyediting import models as copyediting_models
from proofing import models as proofing_models
+from security import logic as security_logic
+from security.const import AccessDeniedMessages as ADM
from security.logic import (
can_edit_file,
can_see_pii,
@@ -33,6 +35,36 @@
logger = get_logger(__name__)
+def review_not_assigned(request):
+ return security_logic.task_not_assigned_message(
+ request.user, ADM.REVIEW_NOT_ASSIGNED
+ )
+
+
+def copyedit_not_assigned(request):
+ return security_logic.task_not_assigned_message(
+ request.user, ADM.COPYEDIT_NOT_ASSIGNED
+ )
+
+
+def proofing_not_assigned(request):
+ return security_logic.task_not_assigned_message(
+ request.user, ADM.PROOFING_NOT_ASSIGNED
+ )
+
+
+def typesetting_not_assigned(request):
+ return security_logic.task_not_assigned_message(
+ request.user, ADM.TYPESETTING_NOT_ASSIGNED
+ )
+
+
+def article_not_yours(request):
+ return security_logic.task_not_assigned_message(
+ request.user, ADM.ARTICLE_NOT_ASSOCIATED
+ )
+
+
# General role-based security decorators
@@ -131,7 +163,7 @@ def wrapper(request, *args, **kwargs):
return func(request, *args, **kwargs)
else:
- deny_access(request)
+ deny_access(request, required_roles=["editor"])
return wrapper
@@ -177,7 +209,10 @@ def wrapper(request, *args, **kwargs):
return func(request, *args, **kwargs)
else:
- deny_access(request)
+ deny_access(
+ request,
+ required_roles=["editor", "production manager", "section editor"],
+ )
return wrapper
@@ -199,7 +234,9 @@ def wrapper(request, *args, **kwargs):
return func(request, *args, **kwargs)
else:
- deny_access(request)
+ deny_access(
+ request, required_roles=["editor", "proofing manager", "section editor"]
+ )
return wrapper
@@ -280,7 +317,7 @@ def wrapper(request, *args, **kwargs):
request.journal
):
return func(request, *args, **kwargs)
- deny_access(request)
+ deny_access(request, required_roles=["editor", "journal manager"])
return wrapper
@@ -316,7 +353,7 @@ def wrapper(request, *args, **kwargs):
deny_access(request, "You are not a section editor for this article")
else:
- deny_access(request)
+ deny_access(request, required_roles=["editor", "journal manager"])
return wrapper
@@ -354,7 +391,7 @@ def wrapper(request, *args, **kwargs):
if request.user.has_an_editor_role(request) or request.user.is_staff:
return func(request, *args, **kwargs)
else:
- deny_access(request)
+ deny_access(request, required_roles=["editor", "section editor"])
return wrapper
@@ -397,7 +434,7 @@ def wrapper(request, *args, **kwargs):
if request.user.is_reviewer(request) or request.user.is_staff:
return func(request, *args, **kwargs)
else:
- deny_access(request)
+ deny_access(request, required_roles=["reviewer"])
return wrapper
@@ -414,7 +451,7 @@ def wrapper(request, *args, **kwargs):
if request.user.is_author(request) or request.user.is_staff:
return func(request, *args, **kwargs)
else:
- deny_access(request)
+ deny_access(request, required_roles=["author"])
return wrapper
@@ -431,10 +468,15 @@ def wrapper(request, *args, **kwargs):
article_id = kwargs["article_id"]
article = models.Article.get_article(request.journal, "id", article_id)
- if request.user.is_author(request) and article.user_is_author(request.user):
- return func(request, *args, **kwargs)
- else:
- deny_access(request)
+ # A missing or cross-journal id is answered like an article the
+ # reader did not write, so ids cannot be enumerated.
+ if not article or not article.user_is_author(request.user):
+ deny_access(request, article_not_yours(request))
+
+ if not request.user.is_author(request):
+ deny_access(request, required_roles=["author"])
+
+ return func(request, *args, **kwargs)
return wrapper
@@ -451,7 +493,7 @@ def wrapper(request, *args, **kwargs):
if request.user.is_proofreader(request) or request.user.is_proofreader(request):
return func(request, *args, **kwargs)
else:
- deny_access(request)
+ deny_access(request, required_roles=["proofreader"])
return wrapper
@@ -468,7 +510,7 @@ def wrapper(request, *args, **kwargs):
if request.user.is_copyeditor(request) or request.user.is_copyeditor(request):
return func(request, *args, **kwargs)
else:
- deny_access(request)
+ deny_access(request, required_roles=["copyeditor"])
return wrapper
@@ -483,18 +525,31 @@ def copyeditor_for_copyedit_required(func):
@base_check_required
def wrapper(request, *args, **kwargs):
copyedit_id = kwargs["copyedit_id"]
- copyedit = get_object_or_404(
- copyediting_models.CopyeditAssignment, pk=copyedit_id
- )
- if (
- request.user == copyedit.copyeditor
- and request.user.is_copyeditor(request)
- or request.user.is_staff
- ):
- return func(request, *args, **kwargs)
- else:
- deny_access(request)
+ # Staff keep access: the views here do not narrow the assignment
+ # to its copyeditor.
+ if request.user.is_staff:
+ if copyediting_models.CopyeditAssignment.objects.filter(
+ pk=copyedit_id,
+ ).exists():
+ return func(request, *args, **kwargs)
+
+ # Scoped to the user and the journal so that somebody else's
+ # assignment, another journal's, and a missing one cannot be
+ # told apart.
+ copyedit = copyediting_models.CopyeditAssignment.objects.filter(
+ pk=copyedit_id,
+ copyeditor=request.user,
+ article__journal=request.journal,
+ ).first()
+
+ if not copyedit:
+ deny_access(request, copyedit_not_assigned(request))
+
+ if not request.user.is_copyeditor(request):
+ deny_access(request, required_roles=["copyeditor"])
+
+ return func(request, *args, **kwargs)
return wrapper
@@ -605,47 +660,40 @@ def wrapper(request, *args, **kwargs):
if access_code is not None:
try:
assignment = review_models.ReviewAssignment.objects.get(
- pk=assignment_id, access_code=access_code
+ pk=assignment_id,
+ access_code=access_code,
+ article__journal=request.journal,
)
if assignment:
return func(request, *args, **kwargs)
else:
- deny_access(request)
+ deny_access(request, ADM.REVIEW_LINK_INVALID.value)
except review_models.ReviewAssignment.DoesNotExist:
- deny_access(request)
+ deny_access(request, ADM.REVIEW_LINK_INVALID.value)
if request.user.is_anonymous or not request.user.is_active:
deny_access(request)
if not request.user.is_reviewer(request):
- deny_access(request)
+ deny_access(request, required_roles=["reviewer"])
try:
- if request.user.is_staff:
- assignment = review_models.ReviewAssignment.objects.get(
- pk=assignment_id
- )
-
- if assignment:
- return func(request, *args, **kwargs)
- else:
- deny_access(request)
-
assignment = review_models.ReviewAssignment.objects.get(
- pk=assignment_id, reviewer=request.user
+ pk=assignment_id,
+ reviewer=request.user,
+ article__journal=request.journal,
)
-
- if assignment:
- if assignment.article.stage not in models.REVIEW_ACCESSIBLE_STAGES:
- deny_access(request)
- else:
- return func(request, *args, **kwargs)
- else:
- deny_access(request)
+ # Somebody else's assignment and a missing one are reported the
+ # same way, so ids cannot be enumerated.
except review_models.ReviewAssignment.DoesNotExist:
- deny_access(request)
+ deny_access(request, review_not_assigned(request))
+
+ if assignment.article.stage not in models.REVIEW_ACCESSIBLE_STAGES:
+ deny_access(request, ADM.REVIEW_STAGE_PASSED.value)
+
+ return func(request, *args, **kwargs)
return wrapper
@@ -729,7 +777,7 @@ def wrapper(request, *args, **kwargs):
return func(request, *args, **kwargs)
else:
- deny_access(request)
+ deny_access(request, required_roles=["editor", "production manager"])
return wrapper
@@ -1114,7 +1162,7 @@ def wrapper(request, *args, **kwargs):
if request.user.is_typesetter(request) or request.user.is_staff:
return func(request, *args, **kwargs)
else:
- deny_access(request)
+ deny_access(request, required_roles=["typesetter"])
return wrapper
@@ -1189,7 +1237,7 @@ def wrapper(request, *args, **kwargs):
if request.user in article.section_editors():
return func(request, *args, **kwargs)
else:
- deny_access(request)
+ deny_access(request, required_roles=["editor", "proofing manager"])
return wrapper
@@ -1284,7 +1332,18 @@ def wrapper(request, *args, **kwargs):
return func(request, *args, **kwargs)
else:
- deny_access(request)
+ task = proofing_models.ProofingTask.objects.filter(
+ pk=kwargs["proofing_task_id"],
+ proofreader=request.user,
+ round__assignment__article__journal=request.journal,
+ ).first()
+
+ if task and task.cancelled:
+ deny_access(request, ADM.TASK_CANCELLED.value)
+ elif task and task.completed:
+ deny_access(request, ADM.TASK_COMPLETED.value)
+ else:
+ deny_access(request, proofing_not_assigned(request))
return wrapper
@@ -1311,7 +1370,18 @@ def wrapper(request, *args, **kwargs):
)
return func(request, *args, **kwargs)
except proofing_models.TypesetterProofingTask.DoesNotExist:
- deny_access(request)
+ task = proofing_models.TypesetterProofingTask.objects.filter(
+ pk=kwargs["typeset_task_id"],
+ typesetter=request.user,
+ proofing_task__round__assignment__article__journal=request.journal,
+ ).first()
+
+ if task and task.cancelled:
+ deny_access(request, ADM.TASK_CANCELLED.value)
+ elif task and task.completed:
+ deny_access(request, ADM.TASK_COMPLETED.value)
+ else:
+ deny_access(request, typesetting_not_assigned(request))
return wrapper
@@ -1416,11 +1486,16 @@ def preprint_manager_wrapper(request, *args, **kwargs):
return preprint_manager_wrapper
-def deny_access(request, *args, **kwargs):
+def deny_access(request, *args, required_roles=None, **kwargs):
"""Wrapper for raising a PermissionDenied exception
- *args and **kwargs are passed to the PermissionDenied constructor
+ *args and **kwargs are passed to the PermissionDenied constructor.
+
+ Callers that pass neither a message nor required_roles keep the bare
+ denial they have always raised.
+
:param request: A django HttpRequest
+ :param required_roles: names of the roles this page asks for, if known
"""
try:
ident = request.user.email
@@ -1435,6 +1510,16 @@ def deny_access(request, *args, **kwargs):
),
)
+ if not args and required_roles:
+ args = (
+ security_logic.access_denied_message(
+ request.user,
+ roles,
+ required_roles=required_roles,
+ journal=getattr(request, "journal", None),
+ ),
+ )
+
raise PermissionDenied(*args, **kwargs)
@@ -1453,7 +1538,7 @@ def review_required_wrapper(request, article_id=None, *args, **kwargs):
article = get_object_or_404(models.Article, pk=article_id)
if article.stage not in models.REVIEW_STAGES:
- deny_access(request)
+ deny_access(request, ADM.REVIEW_STAGE_PASSED.value)
else:
return func(request, article_id, *args, **kwargs)
diff --git a/src/security/logic.py b/src/security/logic.py
index 6a490bac56..d0beb3c2dc 100755
--- a/src/security/logic.py
+++ b/src/security/logic.py
@@ -2,12 +2,77 @@
__author__ = "Martin Paul Eve & Andy Byers"
__license__ = "AGPL v3"
__maintainer__ = "Birkbeck Centre for Technology and Publishing"
+from django.utils.translation import gettext as _
+
from production import models as production_models
from proofing import models as proofing_models
+from security.const import AccessDeniedMessages as ADM
from submission import models as submission_models
from utils import setting_handler
+def task_not_assigned_message(user, message):
+ """Explains that a task belongs to a different account.
+
+ Deliberately says nothing about who the task does belong to, whether it
+ sits on another journal, or whether it exists at all, so that the message
+ cannot be used to discover any of those.
+
+ :param user: the Account making the request
+ :param message: an AccessDeniedMessages member accepting an email
+ """
+ if not getattr(user, "is_authenticated", False):
+ return str(ADM.SIGN_IN_REQUIRED.value)
+
+ return " ".join(
+ [
+ str(message.value) % {"email": user.email},
+ str(ADM.TASK_INVITATION_ADVICE.value),
+ ]
+ )
+
+
+def access_denied_message(user, roles, required_roles=None, journal=None):
+ """Builds the explanation shown to a user who has been denied access.
+
+ Only ever describes the account making the request, never the account a
+ task belongs to, so that the page cannot be used to discover who is
+ working on what.
+
+ The roles a reader holds are recorded per journal, so that part of the
+ message is left out entirely on press and repository pages rather than
+ describing a journal the reader is not looking at.
+
+ :param user: the Account making the request, or an anonymous user
+ :param roles: the AccountRoles the account holds on this journal
+ :param required_roles: names of the roles the page asks for, if known
+ :param journal: the journal in scope, if the request has one
+ """
+ if not getattr(user, "is_authenticated", False):
+ return str(ADM.SIGN_IN_REQUIRED.value)
+
+ parts = [_("You are signed in as %(email)s.") % {"email": user.email}]
+
+ if journal:
+ role_names = sorted({role.role.name for role in roles})
+ if role_names:
+ parts.append(
+ _("On this journal your account has these roles: %(roles)s.")
+ % {"roles": ", ".join(role_names)}
+ )
+ else:
+ parts.append(_("Your account has no roles on this journal."))
+
+ if required_roles:
+ parts.append(
+ _("This page is for users with the %(required)s role.")
+ % {"required": " or ".join(sorted(str(role) for role in required_roles))}
+ )
+
+ parts.append(ADM.WRONG_ACCOUNT_ADVICE.value)
+ return " ".join(str(part) for part in parts)
+
+
def can_edit_file(request, user, file_object, article):
if user.is_anonymous:
return False
diff --git a/src/security/test_security.py b/src/security/test_security.py
index d24698a0b9..b369d1f573 100644
--- a/src/security/test_security.py
+++ b/src/security/test_security.py
@@ -1430,10 +1430,15 @@ def test_reviewer_user_for_assignment_required_decorator_handles_null_user(self)
"reviewer_user_for_assignment_required decorator incorrectly handles request.user=None",
)
- def test_reviewer_user_for_assignment_required_allows_staff(self):
+ def test_reviewer_user_for_assignment_required_blocks_staff(self):
"""
- Tests that reviewer_user_for_assignment_required allows staff to view the article.
- :return: None or raises an assertion
+ Tests that reviewer_user_for_assignment_required holds staff to the
+ same check as anyone else.
+
+ The views behind this decorator fetch the assignment again filtered by
+ reviewer=request.user, so admitting staff here only moved the failure
+ into the view, where it was reported as the review not belonging to
+ them. Staff read reviews through the editor pages instead.
"""
func = Mock()
decorated_func = decorators.reviewer_user_for_assignment_required(func)
@@ -1441,36 +1446,27 @@ def test_reviewer_user_for_assignment_required_allows_staff(self):
request = self.prepare_request_with_user(self.admin_user, self.journal_one)
kwargs = {"assignment_id": self.review_assignment.id}
- decorated_func(request, **kwargs)
+ with self.assertRaises(PermissionDenied):
+ decorated_func(request, **kwargs)
- # test that the callback was called
- self.assertTrue(
+ self.assertFalse(
func.called,
- "reviewer_user_for_assignment_required decorator wrongly prohibits staff from "
- "accessing an article in production",
+ "reviewer_user_for_assignment_required decorator wrongly allows staff "
+ "to open a review assigned to somebody else",
)
- def test_reviewer_user_for_assignment_required_allows_staff_regardless_of_stage(
- self,
- ):
- """
- Tests that reviewer_user_for_assignment_required allows staff to article in review, regardless of stage.
- :return: None or raises an assertion
- """
+ def test_reviewer_user_for_assignment_required_tells_staff_it_is_not_theirs(self):
+ """The message must describe the real reason, not the stage."""
func = Mock()
decorated_func = decorators.reviewer_user_for_assignment_required(func)
request = self.prepare_request_with_user(self.admin_user, self.journal_one)
kwargs = {"assignment_id": self.review_assignment.id}
- decorated_func(request, **kwargs)
+ with self.assertRaises(PermissionDenied) as denial:
+ decorated_func(request, **kwargs)
- # test that the callback was called
- self.assertTrue(
- func.called,
- "reviewer_user_for_assignment_required decorator wrongly prohibits staff from "
- "accessing an article that has been published's production stage",
- )
+ self.assertIn("not assigned to", str(denial.exception))
def test_reviewer_user_for_assignment_required_blocks_editor(self):
"""
@@ -5522,3 +5518,275 @@ def prepare_request_with_user(user, journal=None, press=None, repository=None):
request.repository = repository
return request
+
+
+class AccessDeniedMessageTests(TestCase):
+ """Covers the explanations shown on a 403.
+
+ A reader who follows a task link while signed in with the wrong account
+ used to be told only "Permission denied", with nothing to act on.
+ """
+
+ @classmethod
+ def setUpTestData(cls):
+ cls.press = helpers.create_press()
+ cls.journal_one, cls.journal_two = helpers.create_journals()
+ helpers.create_roles(["editor", "author", "reviewer", "proofreader"])
+
+ cls.reviewer = helpers.create_user(
+ "assigned_reviewer@example.org",
+ roles=["reviewer"],
+ journal=cls.journal_one,
+ )
+ cls.reviewer.is_active = True
+ cls.reviewer.save()
+
+ cls.other_reviewer = helpers.create_user(
+ "other_reviewer@example.org",
+ roles=["reviewer"],
+ journal=cls.journal_one,
+ )
+ cls.other_reviewer.is_active = True
+ cls.other_reviewer.save()
+
+ cls.roleless_user = helpers.create_user("no_roles@example.org")
+ cls.roleless_user.is_active = True
+ cls.roleless_user.save()
+
+ cls.review_assignment = helpers.create_review_assignment(
+ journal=cls.journal_one,
+ reviewer=cls.reviewer,
+ )
+ cls.article = cls.review_assignment.article
+
+ cls.task_owner = helpers.create_user(
+ "task_owner@example.org",
+ roles=["author", "reviewer", "proofreader"],
+ journal=cls.journal_one,
+ )
+ cls.task_owner.is_active = True
+ cls.task_owner.save()
+
+ cls.owned_article = helpers.create_submission(
+ owner=cls.task_owner,
+ journal_id=cls.journal_one.pk,
+ )
+ cls.copyedit = helpers.create_copyedit_assignment(
+ article=cls.owned_article,
+ copyeditor=cls.task_owner,
+ )
+ cls.proofing_task = helpers.create_proofing_task(
+ article=cls.owned_article,
+ proofreader=cls.task_owner,
+ )
+ cls.typeset_task = helpers.create_typesetter_proofing_task(
+ proofing_task=cls.proofing_task,
+ typesetter=cls.task_owner,
+ )
+
+ # The same objects again on the other journal, to prove that a valid
+ # id on a journal the reader is not looking at is answered the same
+ # way as one that does not exist.
+ cls.other_journal_article = helpers.create_submission(
+ owner=cls.task_owner,
+ journal_id=cls.journal_two.pk,
+ )
+ cls.other_journal_copyedit = helpers.create_copyedit_assignment(
+ article=cls.other_journal_article,
+ copyeditor=cls.other_reviewer,
+ )
+ cls.other_journal_review = helpers.create_review_assignment(
+ journal=cls.journal_two,
+ article=cls.other_journal_article,
+ reviewer=cls.other_reviewer,
+ )
+
+ MISSING_ID = 99999
+
+ def get_page(self, user, url_name, kwargs):
+ self.client.force_login(user)
+ return self.client.get(
+ reverse(url_name, kwargs=kwargs),
+ SERVER_NAME=self.journal_one.domain,
+ )
+
+ def assert_indistinguishable(self, user, url_name, key, real_id, other_id):
+ """A wrong owner, another journal and a missing id must look alike.
+
+ Any difference between them, including 404 against 403, lets a reader
+ step through the ids to learn which ones exist.
+ """
+ responses = [
+ self.get_page(user, url_name, {key: real_id}),
+ self.get_page(user, url_name, {key: other_id}),
+ self.get_page(user, url_name, {key: self.MISSING_ID}),
+ ]
+
+ statuses = {response.status_code for response in responses}
+ self.assertEqual(
+ statuses,
+ {403},
+ "{} told the reader which ids exist: {}".format(url_name, statuses),
+ )
+
+ bodies = {self.denial_message(response) for response in responses}
+ self.assertEqual(
+ len(bodies),
+ 1,
+ "{} worded its denials differently: {}".format(url_name, bodies),
+ )
+
+ def denial_message(self, response):
+ content = response.content.decode()
+ for line in content.split("
"):
+ if "is not" in line:
+ return line.split("
")[0].strip()
+ return ""
+
+ def test_copyedit_does_not_reveal_which_ids_exist(self):
+ self.assert_indistinguishable(
+ self.other_reviewer,
+ "do_copyedit",
+ "copyedit_id",
+ self.copyedit.pk,
+ self.other_journal_copyedit.pk,
+ )
+
+ def test_review_does_not_reveal_which_ids_exist(self):
+ self.assert_indistinguishable(
+ self.other_reviewer,
+ "do_review",
+ "assignment_id",
+ self.review_assignment.pk,
+ self.other_journal_review.pk,
+ )
+
+ def test_copyedit_denial_does_not_name_the_copyeditor(self):
+ response = self.get_page(
+ self.other_reviewer,
+ "do_copyedit",
+ {"copyedit_id": self.copyedit.pk},
+ )
+ self.assertNotContains(response, self.task_owner.email, status_code=403)
+
+ def test_proofing_denial_does_not_name_the_proofreader(self):
+ response = self.get_page(
+ self.other_reviewer,
+ "do_proofing",
+ {"proofing_task_id": self.proofing_task.pk},
+ )
+ self.assertEqual(response.status_code, 403)
+ self.assertNotContains(response, self.task_owner.email, status_code=403)
+
+ def test_proofing_hides_completion_from_everyone_but_the_owner(self):
+ """Whether a task is finished is the owner's business alone."""
+ self.proofing_task.completed = timezone.now()
+ self.proofing_task.save()
+
+ response = self.get_page(
+ self.other_reviewer,
+ "do_proofing",
+ {"proofing_task_id": self.proofing_task.pk},
+ )
+ self.assertNotContains(response, "already completed", status_code=403)
+
+ response = self.get_page(
+ self.task_owner,
+ "do_proofing",
+ {"proofing_task_id": self.proofing_task.pk},
+ )
+ self.assertContains(response, "already completed", status_code=403)
+
+ self.proofing_task.completed = None
+ self.proofing_task.save()
+
+ def test_author_task_does_not_reveal_which_articles_exist(self):
+ self.assert_indistinguishable(
+ self.other_reviewer,
+ "review_author_view",
+ "article_id",
+ self.owned_article.pk,
+ self.other_journal_article.pk,
+ )
+
+ def get_review_page(self, user):
+ self.client.force_login(user)
+ return self.client.get(
+ reverse(
+ "do_review",
+ kwargs={"assignment_id": self.review_assignment.pk},
+ ),
+ SERVER_NAME=self.journal_one.domain,
+ )
+
+ def test_message_names_the_account_and_its_roles(self):
+ message = decorators.security_logic.access_denied_message(
+ self.reviewer,
+ list(self.reviewer.accountrole_set.filter(journal=self.journal_one)),
+ journal=self.journal_one,
+ )
+ self.assertIn(self.reviewer.email, message)
+ self.assertIn("reviewer", message)
+
+ def test_message_says_when_the_account_has_no_roles(self):
+ message = decorators.security_logic.access_denied_message(
+ self.roleless_user,
+ [],
+ journal=self.journal_one,
+ )
+ self.assertIn("no roles on this journal", message)
+
+ def test_message_omits_journal_roles_when_there_is_no_journal(self):
+ """Press and repository denials must not describe journal roles."""
+ message = decorators.security_logic.access_denied_message(
+ self.roleless_user,
+ [],
+ required_roles=["editor"],
+ journal=None,
+ )
+ self.assertNotIn("journal", message)
+ self.assertIn(self.roleless_user.email, message)
+
+ def test_message_names_the_role_the_page_requires(self):
+ message = decorators.security_logic.access_denied_message(
+ self.roleless_user,
+ [],
+ required_roles=["editor"],
+ )
+ self.assertIn("editor", message)
+
+ def test_message_asks_anonymous_users_to_sign_in(self):
+ message = decorators.security_logic.access_denied_message(
+ AnonymousUser(),
+ [],
+ )
+ self.assertIn("sign in", message)
+
+ @override_settings(URL_CONFIG="domain")
+ def test_review_for_another_account_explains_the_mismatch(self):
+ response = self.get_review_page(self.other_reviewer)
+ self.assertEqual(response.status_code, 403)
+ self.assertContains(
+ response,
+ "not assigned to {}".format(self.other_reviewer.email),
+ status_code=403,
+ )
+
+ @override_settings(URL_CONFIG="domain")
+ def test_review_for_another_account_does_not_name_the_assignee(self):
+ """The 403 must not disclose who is working on the article."""
+ response = self.get_review_page(self.other_reviewer)
+ self.assertNotContains(
+ response,
+ self.reviewer.email,
+ status_code=403,
+ )
+
+ @override_settings(URL_CONFIG="domain")
+ def test_article_past_review_says_the_review_is_closed(self):
+ self.article.stage = submission_models.STAGE_EDITOR_COPYEDITING
+ self.article.save()
+ response = self.get_review_page(self.reviewer)
+ self.assertContains(response, "moved past the review stage", status_code=403)
+ self.article.stage = submission_models.STAGE_UNDER_REVIEW
+ self.article.save()
diff --git a/src/themes/OLH/templates/403.html b/src/themes/OLH/templates/403.html
index 446aa4527f..9b9aba12a4 100644
--- a/src/themes/OLH/templates/403.html
+++ b/src/themes/OLH/templates/403.html
@@ -1,7 +1,7 @@
{% extends "core/base.html" %}
{% load i18n %}
-{% block page_title %}{% trans "Issue" %} {{ issue }}{% endblock %}
+{% block page_title %}{% trans "Permission Denied" %}{% endblock %}
{% block css %}
{% endblock %}
@@ -14,8 +14,10 @@ {% trans 'Maintenance Mode' %}
{{ request.META.maintenance_mode.value|safe }}
{% else %}
{% trans "Permission Denied" %}
- {% if exception %}
- {{ exception }}
+ {% if exception %}
+ {{ exception }}
+ {% else %}
+ {% trans "You do not have permission to view this page." %}
{% endif %}
{% endif %}
diff --git a/src/themes/material/templates/403.html b/src/themes/material/templates/403.html
new file mode 100644
index 0000000000..dc6ee12c0c
--- /dev/null
+++ b/src/themes/material/templates/403.html
@@ -0,0 +1,22 @@
+{% extends "core/base.html" %}
+{% load i18n %}
+
+{% block page_title %}{% trans "Permission Denied" %}{% endblock %}
+
+{% block body %}
+
+
+ {% if request.META.maintenance_mode %}
+
{% trans "Maintenance Mode" %}
+
{{ request.META.maintenance_mode.value|safe }}
+ {% else %}
+
{% trans "Permission Denied" %}
+ {% if exception %}
+
{{ exception }}
+ {% else %}
+
{% trans "You do not have permission to view this page." %}
+ {% endif %}
+ {% endif %}
+
+
+{% endblock body %}
diff --git a/src/utils/testing/helpers.py b/src/utils/testing/helpers.py
index 755cb1ce89..0dbc120daf 100755
--- a/src/utils/testing/helpers.py
+++ b/src/utils/testing/helpers.py
@@ -32,6 +32,7 @@
from submission import models as sm_models
from review import models as review_models
from copyediting import models as copyediting_models
+from proofing import models as proofing_models
from comms import models as comms_models
from cms import models as cms_models
from utils import setting_handler, models as utils_models
@@ -898,3 +899,34 @@ def send_contact_message(
if contact_form.is_valid():
core_logic.send_contact_message(contact_form, request)
return utils_models.LogEntry.objects.order_by("-date").first()
+
+
+def create_proofing_task(article, proofreader, manager=None, **kwargs):
+ """Builds the assignment, round and task a proofreader is given."""
+ if not manager:
+ manager = create_editor(article.journal)
+
+ assignment, _created = proofing_models.ProofingAssignment.objects.get_or_create(
+ article=article,
+ defaults={"proofing_manager": manager, "editor": manager},
+ )
+ proofing_round = proofing_models.ProofingRound.objects.create(
+ assignment=assignment,
+ number=assignment.current_proofing_round_number + 1,
+ )
+ return proofing_models.ProofingTask.objects.create(
+ round=proofing_round,
+ proofreader=proofreader,
+ due=timezone.now() + datetime.timedelta(days=3),
+ **kwargs,
+ )
+
+
+def create_typesetter_proofing_task(proofing_task, typesetter, **kwargs):
+ """Builds the corrections task a typesetter is given after proofing."""
+ return proofing_models.TypesetterProofingTask.objects.create(
+ proofing_task=proofing_task,
+ typesetter=typesetter,
+ due=timezone.now() + datetime.timedelta(days=3),
+ **kwargs,
+ )