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, + )