For help with Janeway, contact --No support email set--.
" @@ -23,3 +42,44 @@ def test_render_nested_settings(self): nested_settings=[('support_email','general')], ) self.assertEqual(expected_rendered_setting, rendered_setting) + + @patch('core.logic.reverse') + def test_reverse_with_next_in_get_request(self, mock_reverse): + mock_reverse.return_value = '/my/path/?my=params' + self.request.GET = {'next': self.next_url_encoded} + reversed_url = logic.reverse_with_next('/test/', self.request) + self.assertIn(self.next_url_encoded, reversed_url) + + @patch('core.logic.reverse') + def test_reverse_with_next_in_post_request(self, mock_reverse): + mock_reverse.return_value = '/my/path/?my=params' + self.request.POST = {'next': self.next_url_encoded} + reversed_url = logic.reverse_with_next('/test/', self.request) + self.assertIn(self.next_url_encoded, reversed_url) + + @patch('core.logic.reverse') + def test_reverse_with_next_in_kwarg(self, mock_reverse): + mock_reverse.return_value = '/my/path/?my=params' + reversed_url = logic.reverse_with_next( + '/test/', + self.request, + next_url=self.next_url_encoded, + ) + self.assertIn(self.next_url_encoded, reversed_url) + + @patch('core.logic.reverse') + def test_reverse_with_next_no_next(self, mock_reverse): + mock_reverse.return_value = '/my/url/?my=params' + reversed_url = logic.reverse_with_next('/test/', self.request) + self.assertEqual(mock_reverse.return_value, reversed_url) + + def test_get_confirm_account_url(self): + url = logic.get_confirm_account_url( + self.request, + self.inactive_user, + next_url=self.next_url_encoded, + ) + self.assertIn( + f'/register/step/2/8bd3cdc9-1c3c-4ec9-99bc-9ea0b86a3c55/?next={ self.next_url_encoded }', + url, + ) diff --git a/src/core/tests/test_views.py b/src/core/tests/test_views.py new file mode 100644 index 0000000000..9b0db8a928 --- /dev/null +++ b/src/core/tests/test_views.py @@ -0,0 +1,369 @@ +__copyright__ = "Copyright 2024 Birkbeck, University of London" +__author__ = "Open Library of Humanities" +__license__ = "AGPL v3" +__maintainer__ = "Open Library of Humanities" + +from mock import patch +from uuid import uuid4 + +from django.test import Client, TestCase, override_settings + +from utils.testing import helpers + +from core import models as core_models + + +class NextURLTests(TestCase): + + @classmethod + def setUpTestData(cls): + cls.press = helpers.create_press() + cls.journal_one, cls.journal_two = helpers.create_journals() + cls.theme_dirs = helpers.get_theme_dirs() + helpers.create_roles(['author']) + cls.user_email = 'sukv8golcvwervs0y7e5@example.org' + cls.user_password = 'xUMXW1oXn2l8L26Kixi2' + cls.user = core_models.Account.objects.create_user( + cls.user_email, + password=cls.user_password, + ) + cls.user.is_active = True + cls.user_orcid = 'https://orcid.org/0000-0001-2345-6789' + cls.user.orcid = cls.user_orcid + cls.orcid_token_str = uuid4() + cls.orcid_token = core_models.OrcidToken.objects.create( + token=cls.orcid_token_str, + orcid=cls.user_orcid, + ) + cls.reset_token_str = uuid4() + cls.reset_token = core_models.PasswordResetToken.objects.create( + account=cls.user, + token=cls.reset_token_str, + ) + cls.user.save() + + # The raw unicode string of a 'next' URL + cls.next_url_raw = '/target/page/?a=b&x=y' + + # The unicode string url-encoded with safe='/' + cls.next_url_encoded = '/target/page/%3Fa%3Db%26x%3Dy' + + # The unicode string url-encoded with safe='' + cls.next_url_encoded_no_safe = '%2Ftarget%2Fpage%2F%3Fa%3Db%26x%3Dy' + + # next_url_encoded with its 'next' key + cls.next_url_query_string = 'next=/target/page/%3Fa%3Db%26x%3Dy' + + # The core_login url with encoded next url + cls.core_login_with_next = '/login/?next=/target/page/%3Fa%3Db%26x%3Dy' + + def setUp(self): + self.client = Client() + + +class UserLoginTests(NextURLTests): + + def test_is_authenticated_redirects_to_next(self): + self.client.login(username=self.user_email, password=self.user_password) + data = { + 'next': self.next_url_raw, + } + response = self.client.get('/login/', data, follow=True) + self.assertIn((self.next_url_raw, 302), response.redirect_chain) + + @patch('core.views.authenticate') + def test_login_success_redirects_to_next(self, authenticate): + authenticate.return_value = self.user + data = { + 'user_name': self.user_email, + 'user_pass': self.user_password, + 'next': self.next_url_raw, + } + response = self.client.post('/login/', data, follow=True) + self.assertIn((self.next_url_raw, 302), response.redirect_chain) + + @patch('utils.template_override_middleware.Loader.get_theme_dirs') + @override_settings(ENABLE_OIDC=True) + def test_oidc_link_has_next(self, get_theme_dirs): + data = { + 'next': self.next_url_raw, + } + for theme_dirs in self.theme_dirs: + get_theme_dirs.return_value = theme_dirs + response = self.client.get('/login/', data) + self.assertIn( + f'/oidc/authenticate/?next={self.next_url_encoded}', + response.content.decode(), + ) + + @patch('utils.template_override_middleware.Loader.get_theme_dirs') + @override_settings(ENABLE_ORCID=True) + def test_orcid_link_has_next(self, get_theme_dirs): + data = { + 'next': self.next_url_raw, + } + for theme_dirs in self.theme_dirs: + get_theme_dirs.return_value = theme_dirs + response = self.client.get('/login/', data) + self.assertIn( + f'/login/orcid/?next={self.next_url_encoded}', + response.content.decode(), + ) + + @patch('utils.template_override_middleware.Loader.get_theme_dirs') + def test_forgot_password_link_has_next(self, get_theme_dirs): + data = { + 'next': self.next_url_raw, + } + for theme_dirs in self.theme_dirs: + get_theme_dirs.return_value = theme_dirs + response = self.client.get('/login/', data) + self.assertIn( + f'/reset/step/1/?next={self.next_url_encoded}', + response.content.decode(), + ) + + @patch('utils.template_override_middleware.Loader.get_theme_dirs') + def test_register_link_has_next(self, get_theme_dirs): + data = { + 'next': self.next_url_raw, + } + for theme_dirs in self.theme_dirs: + get_theme_dirs.return_value = theme_dirs + response = self.client.get('/login/', data) + self.assertIn( + f'/register/step/1/?next={self.next_url_encoded}', + response.content.decode(), + ) + + +class UserLoginOrcidTests(NextURLTests): + + @override_settings(ENABLE_ORCID=False) + def test_orcid_disabled_redirects_with_next(self): + data = { + 'next': self.next_url_raw, + } + response = self.client.get('/login/orcid/', data, follow=True) + self.assertIn(self.next_url_encoded, response.redirect_chain[0][0]) + + @override_settings(ENABLE_ORCID=True) + def test_no_orcid_code_redirects_with_next(self): + data = { + 'next': self.next_url_raw, + } + response = self.client.get('/login/orcid/', data) + self.assertIn(self.next_url_encoded_no_safe, response.url) + + @patch('core.views.orcid.retrieve_tokens') + @override_settings(ENABLE_ORCID=True) + def test_orcid_id_account_found_redirects_to_next( + self, + retrieve_tokens, + ): + retrieve_tokens.return_value = self.user_orcid + data = { + 'code': '12345', + 'next': self.next_url_raw, + } + response = self.client.get('/login/orcid/', data, follow=True) + self.assertIn((self.next_url_raw, 302), response.redirect_chain) + + @patch('core.views.orcid.get_orcid_record_details') + @patch('core.views.orcid.retrieve_tokens') + @override_settings(ENABLE_ORCID=True) + def test_orcid_id_no_account_matching_email_redirects_to_next( + self, + retrieve_tokens, + orcid_details, + ): + # Change ORCID so it doesn't work + retrieve_tokens.return_value = 'https://orcid.org/0000-0001-2312-3123' + + # Return an email that will work + orcid_details.return_value = {'emails': [self.user_email]} + + data = { + 'code': '12345', + 'state': self.next_url_raw, + } + response = self.client.get('/login/orcid/', data, follow=True) + self.assertIn((self.next_url_raw, 302), response.redirect_chain) + + @patch('core.views.orcid.get_orcid_record_details') + @patch('core.views.orcid.retrieve_tokens') + @override_settings(ENABLE_ORCID=True) + def test_orcid_id_no_account_no_matching_email_redirects_to_next( + self, + retrieve_tokens, + orcid_details, + ): + # Change ORCID so it doesn't work + retrieve_tokens.return_value = 'https://orcid.org/0000-0001-2312-3123' + + orcid_details.return_value = {'emails': []} + data = { + 'code': '12345', + 'next': self.next_url_raw, + } + response = self.client.get('/login/orcid/', data, follow=True) + self.assertIn( + self.next_url_query_string, + response.redirect_chain[0][0], + ) + + @patch('core.views.orcid.retrieve_tokens') + @override_settings(ENABLE_ORCID=True) + def test_no_orcid_id_retrieved_redirects_with_next(self, retrieve_tokens): + retrieve_tokens.return_value = None + data = { + 'code': '12345', + 'next': self.next_url_raw, + } + response = self.client.get('/login/orcid/', data, follow=True) + self.assertIn((self.core_login_with_next, 302), response.redirect_chain) + + +class GetResetTokenTests(NextURLTests): + + @patch('core.views.logic.start_reset_process') + def test_start_reset_redirects_with_next(self, _start_reset): + data = { + 'email_address': self.user_email, + 'next': self.next_url_raw, + } + response = self.client.post('/reset/step/1/', data, follow=True) + self.assertIn((self.core_login_with_next, 302), response.redirect_chain) + + +class ResetPasswordTests(NextURLTests): + + @patch('core.views.logic.password_policy_check') + def test_reset_password_form_valid_redirects_with_next(self, password_check): + password_check.return_value = None + data = { + 'password_1': 'qsX1roLama3ADotEopfq', + 'password_2': 'qsX1roLama3ADotEopfq', + 'next': self.next_url_raw, + } + reset_step_2_path = f'/reset/step/2/{self.reset_token.token}/' + response = self.client.post(reset_step_2_path, data, follow=True) + self.assertIn((self.core_login_with_next, 302), response.redirect_chain) + + +class RegisterTests(NextURLTests): + + @patch('core.views.logic.password_policy_check') + @override_settings(CAPTCHA_TYPE='') + @override_settings(ENABLE_ORCID=True) + def test_register_email_form_valid_redirects_with_next(self, password_check): + password_check.return_value = None + data = { + 'email': 'kjhsaqccxf7qfwirhqia@example.org', + 'password_1': 'qsX1roLama3ADotEopfq', + 'password_2': 'qsX1roLama3ADotEopfq', + 'first_name': 'New', + 'last_name': 'User', + 'next': self.next_url_raw, + } + response = self.client.post('/register/step/1/', data, follow=True) + self.assertIn((self.core_login_with_next, 302), response.redirect_chain) + + @patch('core.views.orcid.get_orcid_record_details') + @patch('core.views.logic.password_policy_check') + @override_settings(CAPTCHA_TYPE='') + @override_settings(ENABLE_ORCID=True) + def test_user_register_orcid_form_valid_redirects_to_next( + self, + password_check, + get_orcid_details + ): + get_orcid_details.return_value = { + 'first_name': 'New', + 'last_name': 'User', + 'emails': ['kjhsaqccxf7qfwirhqia@example.org'], + } + password_check.return_value = None + data = { + 'first_name': 'New', + 'last_name': 'User', + 'email': 'kjhsaqccxf7qfwirhqia@example.org', + 'password_1': 'qsX1roLama3ADotEopfq', + 'password_2': 'qsX1roLama3ADotEopfq', + 'token': self.orcid_token_str, + 'next': self.next_url_raw, + } + response = self.client.post('/register/step/1/', data, follow=True) + self.assertIn((self.next_url_raw, 302), response.redirect_chain) + + +class OrcidRegistrationTests(NextURLTests): + + @patch('utils.template_override_middleware.Loader.get_theme_dirs') + def test_login_link_has_next(self, get_theme_dirs): + data = { + 'next': self.next_url_raw, + } + for theme_dirs in self.theme_dirs: + get_theme_dirs.return_value = theme_dirs + orcid_registration_path = f'/register/step/orcid/{self.orcid_token_str}/' + response = self.client.get(orcid_registration_path, data) + self.assertIn( + f'/login/?next={self.next_url_encoded}', + response.content.decode(), + ) + + @patch('utils.template_override_middleware.Loader.get_theme_dirs') + def test_forgot_password_link_has_next(self, get_theme_dirs): + data = { + 'next': self.next_url_raw, + } + for theme_dirs in self.theme_dirs: + get_theme_dirs.return_value = theme_dirs + orcid_registration_path = f'/register/step/orcid/{self.orcid_token_str}/' + response = self.client.get(orcid_registration_path, data) + self.assertIn( + f'/reset/step/1/?next={self.next_url_encoded}', + response.content.decode(), + ) + + @patch('utils.template_override_middleware.Loader.get_theme_dirs') + def test_register_link_has_next(self, get_theme_dirs): + data = { + 'next': self.next_url_raw, + } + for theme_dirs in self.theme_dirs: + get_theme_dirs.return_value = theme_dirs + orcid_registration_path = f'/register/step/orcid/{self.orcid_token_str}/' + response = self.client.get(orcid_registration_path, data) + self.assertIn( + f'/register/step/1/?next={self.next_url_encoded}&token={self.orcid_token_str}', + response.content.decode(), + ) + + +class ActivateAccountTests(NextURLTests): + + @patch('core.views.models.Account.objects.get') + def test_activate_success_redirects_to_next(self, objects_get): + objects_get.return_value = self.user + data = { + 'next': self.next_url_raw, + } + response = self.client.post('/register/step/2/12345/', data, follow=True) + self.assertIn((self.core_login_with_next, 302), response.redirect_chain) + + @patch('utils.template_override_middleware.Loader.get_theme_dirs') + @patch('core.views.models.Account.objects.get') + def test_login_link_has_next(self, objects_get, get_theme_dirs): + objects_get.return_value = None + data = { + 'next': self.next_url_raw, + } + for theme_dirs in self.theme_dirs: + get_theme_dirs.return_value = theme_dirs + response = self.client.get('/register/step/2/12345/', data) + self.assertIn( + self.core_login_with_next, + response.content.decode(), + ) diff --git a/src/core/views.py b/src/core/views.py index c5ad018bcf..f42b7aae6e 100755 --- a/src/core/views.py +++ b/src/core/views.py @@ -6,6 +6,7 @@ from importlib import import_module import json +from urllib.parse import unquote, urlencode import pytz import time @@ -64,10 +65,11 @@ def user_login(request): :param request: HttpRequest :return: HttpResponse """ + next_url = request.GET.get('next', '') or request.POST.get('next', '') if request.user.is_authenticated: messages.info(request, 'You are already logged in.') - if request.GET.get('next'): - return redirect(request.GET.get('next')) + if next_url: + return redirect(next_url) else: return redirect(reverse('website_index')) else: @@ -87,10 +89,10 @@ def user_login(request): form = forms.LoginForm(request.POST, bad_logins=bad_logins) if form.is_valid(): - user = request.POST.get('user_name').lower() - pawd = request.POST.get('user_pass') + username = request.POST.get('user_name').lower() + password = request.POST.get('user_pass') - user = authenticate(username=user, password=pawd) + user = authenticate(username=username, password=password) if user is not None: login(request, user) @@ -107,8 +109,8 @@ def user_login(request): except models.OrcidToken.DoesNotExist: pass - if request.GET.get('next'): - return redirect(request.GET.get('next')) + if next_url: + return redirect(next_url) elif request.journal: return redirect(reverse('core_dashboard')) else: @@ -148,29 +150,61 @@ def user_login(request): def user_login_orcid(request): """ - Allows a user to login with ORCiD + Allows a user to login with ORCiD. + Redirects them to the Janeway login page if ORCID is not enabled. + Sends them to orcid.org for authorization if needed. + Logs them in when returned here with the right details. :param request: HttpRequest object :return: HttpResponse object """ - orcid_code = request.GET.get('code', None) action = request.GET.get('state', 'login') + orcid_code = request.GET.get('code', '') or request.POST.get('code', '') + state = request.GET.get('state', '') or request.POST.get('state', '') + next_url = state or request.GET.get('next', '') or request.POST.get('next', '') - if orcid_code and django_settings.ENABLE_ORCID: + if not django_settings.ENABLE_ORCID: + messages.add_message( + request, + messages.WARNING, + _('ORCID is not enabled.' + 'Please log in with your username and password.') + ) + return redirect( + logic.reverse_with_next('core_login', request, next_url=next_url) + ) + + elif not orcid_code: + messages.add_message( + request, + messages.WARNING, + _('No authorisation code provided.' + 'Please try again or log in with your username and password.') + ) + base = django_settings.ORCID_URL + query_dict = { + 'client_id': django_settings.ORCID_CLIENT_ID, + 'response_type': 'code', + 'scope': '/authenticate', + 'redirect_uri': orcid.build_redirect_uri(request.site_type), + 'state': next_url, + } + orcid_login_url = f'{base}?{urlencode(query_dict)}' + return redirect(orcid_login_url) + + else: orcid_id = orcid.retrieve_tokens( orcid_code, request.site_type, action=action ) - if orcid_id: try: user = models.Account.objects.get(orcid=orcid_id) if action == 'login': user.backend = 'django.contrib.auth.backends.ModelBackend' login(request, user) - - if request.GET.get('next'): - return redirect(request.GET.get('next')) + if next_url: + return redirect(next_url) elif request.journal: return redirect(reverse('core_dashboard')) else: @@ -185,8 +219,8 @@ def user_login_orcid(request): # Store ORCID for future authentication requests candidates.update(orcid=orcid_id) login(request, candidates.first()) - if request.GET.get('next'): - return redirect(request.GET.get('next')) + if next_url: + return redirect(next_url) elif request.journal: return redirect(reverse('core_dashboard')) else: @@ -199,20 +233,24 @@ def user_login_orcid(request): if action == 'register': return redirect(reverse('core_register') + f'?token={new_token.token}') else: - return redirect(reverse('core_orcid_registration', kwargs={'token': new_token.token})) + return redirect( + logic.reverse_with_next( + 'core_orcid_registration', + request, + next_url=next_url, + kwargs={'token': new_token.token} + ) + ) else: messages.add_message( request, messages.WARNING, - 'Valid ORCiD not returned, please try again, or login with your username and password.' + 'Valid ORCiD not returned. ' + 'Please try again, or log in with your username and password.' + ) + return redirect( + logic.reverse_with_next('core_login', request, next_url=next_url) ) - return redirect(reverse('core_login')) - else: - messages.add_message( - request, - messages.WARNING, - 'No authorisation code provided, please try again or login with your username and password.') - return redirect(reverse('core_login')) @login_required @@ -250,9 +288,9 @@ def get_reset_token(request): try: account = models.Account.objects.get(email__iexact=email_address) logic.start_reset_process(request, account) - return redirect(reverse('core_login')) + return redirect(logic.reverse_with_next('core_login', request)) except models.Account.DoesNotExist: - return redirect(reverse('core_login')) + return redirect(logic.reverse_with_next('core_login', request)) template = 'core/accounts/get_reset_token.html' context = { @@ -265,7 +303,9 @@ def get_reset_token(request): def reset_password(request, token): """ - Takes a reset token and checks if it is valid then allows a user to reset their password, adter it expires the token + Takes a reset token and checks if it is valid. + Then it allows a user to reset their password, + and finally it expires the token. :param request: HttpRequest :param token: string, PasswordResetToken.token :return: HttpResponse object @@ -294,7 +334,7 @@ def reset_password(request, token): reset_token.expired = True reset_token.save() messages.add_message(request, messages.SUCCESS, 'Your password has been reset.') - return redirect(reverse('core_login')) + return redirect(logic.reverse_with_next('core_login', request)) template = 'core/accounts/reset_password.html' context = { @@ -307,14 +347,19 @@ def reset_password(request, token): def register(request): """ - Displays a form for users to register with the journal. If the user is registering on a journal we give them + Displays a form for users to register with the journal. + If the user is registering on a journal we give them the Author role. :param request: HttpRequest object :return: HttpResponse object """ context = {} initial = {} - token, token_obj = request.GET.get('token', None), None + + token = request.GET.get('token') or request.POST.get('token') + token_obj = None + next_url = request.GET.get('next', '') or request.POST.get('next', '') + if token: token_obj = get_object_or_404(models.OrcidToken, token=token) orcid_details = orcid.get_orcid_record_details(token_obj.orcid) @@ -358,8 +403,8 @@ def register(request): new_user.is_active = True new_user.save() login(request, new_user) - if request.GET.get('next'): - return redirect(request.GET.get('next')) + if next_url: + return redirect(next_url) elif request.journal: return redirect(reverse('core_dashboard')) else: @@ -373,10 +418,10 @@ def register(request): messages.add_message( request, messages.SUCCESS, - _('Your account has been created, please follow the' + _('Your account has been created. Please follow the ' 'instructions in the email that has been sent to you.'), ) - return redirect(reverse('core_login')) + return redirect(logic.reverse_with_next('core_login', request)) template = 'core/accounts/register.html' context["form"] = form @@ -419,7 +464,7 @@ def activate_account(request, token): _('Account activated'), ) - return redirect(reverse('core_login')) + return redirect(logic.reverse_with_next('core_login', request)) template = 'core/accounts/activate_account.html' context = { @@ -459,7 +504,8 @@ def edit_profile(request): try: validate_email(email_address) try: - logic.handle_email_change(request, email_address) + next_url = reverse('core_edit_profile') + logic.handle_email_change(request, email_address, next_url=next_url) return redirect(reverse('website_index')) except IntegrityError: messages.add_message( diff --git a/src/journal/models.py b/src/journal/models.py index 1e7c44e33e..25661861a1 100644 --- a/src/journal/models.py +++ b/src/journal/models.py @@ -7,6 +7,7 @@ from itertools import chain from operator import itemgetter import collections +from urllib.parse import parse_qs import uuid import os import re diff --git a/src/journal/views.py b/src/journal/views.py index f595254f72..69e518d153 100755 --- a/src/journal/views.py +++ b/src/journal/views.py @@ -43,10 +43,20 @@ from metrics.logic import store_article_access from review import forms as review_forms, models as review_models from submission import encoding -from security.decorators import article_stage_accepted_or_later_required, \ - article_stage_accepted_or_later_or_staff_required, article_exists, file_user_required, has_request, has_journal, \ - file_history_user_required, file_edit_user_required, production_user_or_editor_required, \ - editor_user_required, keyword_page_enabled +from security.decorators import ( + article_exists, + article_stage_accepted_or_later_required, + article_stage_accepted_or_later_or_staff_required, + editor_user_required, + editor_user_required_and_can_see_pii, + file_edit_user_required, + file_history_user_required, + file_user_required, + has_journal, + has_request, + keyword_page_enabled, + production_user_or_editor_required, +) from submission import models as submission_models from utils import models as utils_models, shared, setting_handler from utils.logger import get_logger @@ -838,6 +848,7 @@ def submit_files_info(request, article_id, file_id): @login_required @file_history_user_required +@editor_user_required_and_can_see_pii def file_history(request, article_id, file_id): """ Renders a template to show the history of a file. @@ -2432,7 +2443,7 @@ def texture_edit(request, file_id): return render(request, template, context) -@editor_user_required +@editor_user_required_and_can_see_pii def document_management(request, article_id): document_article = get_object_or_404( submission_models.Article, diff --git a/src/press/forms.py b/src/press/forms.py index cfcbe5e5db..f035ce5f9c 100755 --- a/src/press/forms.py +++ b/src/press/forms.py @@ -39,8 +39,6 @@ class Meta: 'password_number', 'password_upper', 'password_length', - 'password_reset_text', - 'registration_text', 'tracking_code', 'disable_journals', 'privacy_policy_url', @@ -51,8 +49,6 @@ class Meta: ), 'footer_description': TinyMCE(), 'journal_footer_text': TinyMCE(), - 'password_reset_text': TinyMCE(), - 'registration_text': TinyMCE(), 'description': TinyMCE(), } diff --git a/src/press/models.py b/src/press/models.py index 20c19ff192..60ff035090 100755 --- a/src/press/models.py +++ b/src/press/models.py @@ -129,8 +129,6 @@ class Press(AbstractSiteModel): help_text="URL to an external privacy-policy, linked from the page" " footer. If blank, it links to the Janeway CMS page: /site/privacy.", ) - password_reset_text = JanewayBleachField(blank=True, null=True, default=press_text('reset')) - registration_text = JanewayBleachField(blank=True, null=True, default=press_text('registration')) password_number = models.BooleanField(default=False, help_text='If set, passwords must include one number.') password_upper = models.BooleanField(default=False, help_text='If set, passwords must include one upper case.') diff --git a/src/security/decorators.py b/src/security/decorators.py index 5112e58de5..a73994a609 100755 --- a/src/security/decorators.py +++ b/src/security/decorators.py @@ -18,7 +18,13 @@ from submission import models from copyediting import models as copyediting_models from proofing import models as proofing_models -from security.logic import can_edit_file, can_view_file_history, can_view_file, is_data_figure_file +from security.logic import ( + can_edit_file, + can_see_pii, + can_view_file, + can_view_file_history, + is_data_figure_file, +) from utils import setting_handler from utils.logger import get_logger from repository import models as preprint_models @@ -43,10 +49,13 @@ def base_check(request, login_redirect=False): ): if login_redirect is True: request_params = request.GET.urlencode() - params = urlencode({"next": f"{request.path}?{request_params}"}) + if request_params: + params = urlencode({"next": f"{request.path}?{request_params}"}) + else: + params = urlencode({"next": request.path}) return redirect('{0}?{1}'.format(reverse('core_login'), params)) elif isinstance(login_redirect, str): - params = urlencode({"next": redirect}) + params = urlencode({"next": login_redirect}) return redirect('{0}?{1}'.format(reverse('core_login'), params)) else: return False @@ -269,7 +278,6 @@ def editor_user_required(func): @base_check_required def wrapper(request, *args, **kwargs): - article_id = kwargs.get('article_id', None) if request.user.is_editor(request) or request.user.is_staff or request.user.is_journal_manager(request.journal): @@ -288,6 +296,25 @@ def wrapper(request, *args, **kwargs): return wrapper +def editor_user_required_and_can_see_pii(func): + """Extends editor_user_required to check if SE can see PII""" + @editor_user_required + def can_see_pii_decorator(request, *args, **kwargs): + article_id = kwargs.get('article_id') + article = get_object_or_404(models.Article, pk=article_id) + if ( + request.user in article.section_editors() + and not can_see_pii(request, article) + ): + deny_access( + request, + "You cannot access this page yet, because it could reveal" + " personally identifiable information." + ) + return func(request, *args, **kwargs) + return can_see_pii_decorator + + def any_editor_user_required(func): """Checks if the user is any type of editor or otherwise is a staff member. diff --git a/src/security/logic.py b/src/security/logic.py index 2afb4b21d7..277b7c4299 100755 --- a/src/security/logic.py +++ b/src/security/logic.py @@ -5,6 +5,7 @@ from production import models as production_models from proofing import models as proofing_models from submission import models as submission_models +from utils import setting_handler def can_edit_file(request, user, file_object, article): @@ -125,3 +126,29 @@ def is_data_figure_file(file_object, article_object): # deny access to all others return False + + +def can_see_pii(request, article): + # Before doing anything, check the setting is enabled: + se_pii_filter_enabled = setting_handler.get_setting( + setting_group_name='permission', + setting_name='se_pii_filter', + journal=article.journal, + ).processed_value + + # early return if filter not enabled + if not se_pii_filter_enabled: + return True + + # Check if the user is an SE and return an anonymised value. + # If the user is not a section editor we assume they have permission + # to view the actual value. + stages = [ + submission_models.STAGE_UNASSIGNED, + submission_models.STAGE_ASSIGNED, + submission_models.STAGE_UNDER_REVIEW, + submission_models.STAGE_UNDER_REVISION, + ] + if request.user in article.section_editors() and article.stage in stages: + return False + return True diff --git a/src/security/templatetags/securitytags.py b/src/security/templatetags/securitytags.py index 5f5d6fcb7a..52599950c2 100755 --- a/src/security/templatetags/securitytags.py +++ b/src/security/templatetags/securitytags.py @@ -1,5 +1,8 @@ from django import template + +from core.middleware import GlobalRequestMiddleware from security import logic +from django.utils.translation import gettext_lazy as _ register = template.Library() @@ -87,3 +90,26 @@ def is_repository_manager(context): def is_preprint_editor(context): request = context['request'] return request.user.is_preprint_editor(request) + + +@register.filter +def se_can_see_pii(value, article): + request = GlobalRequestMiddleware.get_current_request() + + if logic.can_see_pii(request, article): + return value + else: + return _('[Anonymised data]') + + +@register.simple_tag(takes_context=True) +def can_see_pii_tag(context, article): + request = context.get('request') + + if logic.can_see_pii(request, article): + return True + else: + return False + + + diff --git a/src/security/test_security.py b/src/security/test_security.py index 2510b9a905..c1ab2e72b2 100644 --- a/src/security/test_security.py +++ b/src/security/test_security.py @@ -29,7 +29,7 @@ from press import models as press_models from utils.install import update_xsl_files, update_settings from utils import setting_handler -from utils.testing import helpers +from utils.testing import context_managers, helpers class TestSecurity(TestCase): @@ -3104,6 +3104,23 @@ def test_section_editor_cant_access_random_article(self): with self.assertRaises(PermissionDenied): decorated_func(request, **kwargs) + def test_section_editor_cant_access_view_because_of_pii(self): + func = Mock() + + with context_managers.janeway_setting_override( + "permission", "se_pii_filter", self.journal_one, True, + ): + decorated_func = decorators.editor_user_required_and_can_see_pii(func) + kwargs = {'article_id': self.article_in_review.pk} + + request = self.prepare_request_with_user(self.section_editor, self.journal_one) + + with self.assertRaises( + PermissionDenied, + msg="SE PII filter not leading to PermissionDenied", + ): + decorated_func(request, **kwargs) + def test_article_stage_review_required_with_review_article(self): func = Mock() decorated_func = decorators.article_stage_review_required(func) @@ -3967,19 +3984,105 @@ def test_user_has_completed_review_for_article_granted(self): "permission denied.", ) + # PII Tests + def test_section_editor_cannot_see_pii_when_enabled(self): + """ + Test that the section editor cannot see PII and it is anonymized in + the response. + """ + with context_managers.janeway_setting_override( + "permission", "se_pii_filter", self.article_in_review.journal, True, + ): + self.client.force_login(self.section_editor) + article_views = [ + 'manage_article_log', + 'edit_metadata', + 'review_unassigned_article', + 'review_in_review', + 'review_decision', + 'decision_helper', + ] + general_views = [ + 'core_dashboard', + 'review_home', + 'core_active_submissions', + 'review_unassigned', + ] + list_of_pii_strings = self.get_pii_strings_for_article( + self.article_in_review, + ) + + for view_name in general_views: + response = self.client.get( + reverse( + view_name, + ), + SERVER_NAME=self.article_in_review.journal.domain, + ) + found_strings = [ + string in response.content.decode('utf-8') for string in + list_of_pii_strings + ] + self.assertFalse(any(found_strings)) + + for view_name in article_views: + kwargs = { + 'article_id': self.article_in_review.pk, + } + if view_name == 'review_decision': + kwargs['decision'] = 'accept' + response = self.client.get( + reverse( + view_name, + kwargs=kwargs, + ), + SERVER_NAME=self.article_in_review.journal.domain, + ) + found_strings = [ + string in response.content.decode('utf-8') for string in + list_of_pii_strings + ] + self.assertFalse(any(found_strings)) + # General helper functions @staticmethod - def create_user(username, roles=None, journal=None): + def create_user( + username, + roles=None, + journal=None, + first_name='', + last_name='', + institution='', + department='', + orcid='', + country_name='', + country_code='', + ): """ Creates a user with the specified permissions. :return: a user with the specified permissions """ # For consistency, outsourced to newer testing helpers + country_obj = None + if country_code and country_name: + country_obj, c = core_models.Country.objects.get_or_create( + code=country_code, + name=country_name, + ) + return helpers.create_user( username, roles=roles, journal=journal, + **{ + 'first_name': first_name, + 'last_name': last_name, + 'institution': institution, + 'department': department, + 'orcid': orcid, + 'country': country_obj, + } ) @staticmethod @@ -4007,6 +4110,28 @@ def create_journals(): return journal_one, journal_two + @staticmethod + def get_pii_strings_for_article(article): + pii_strings = [] + for fa in article.frozen_authors(): + pii_strings.append(fa.first_name) + pii_strings.append(fa.last_name) + pii_strings.append(fa.email) + pii_strings.append(fa.orcid if fa.orcid else '') + pii_strings.append(fa.institution) + pii_strings.append(fa.department) + pii_strings.append(fa.country) + pii_strings.append(article.correspondence_author.first_name) + pii_strings.append(article.correspondence_author.last_name) + pii_strings.append(article.correspondence_author.email) + pii_strings.append(article.correspondence_author.department) + pii_strings.append( + article.correspondence_author.orcid if article.correspondence_author.orcid else '' + ) + pii_strings.append(article.correspondence_author.institution) + pii_strings.append(article.correspondence_author.country) + return [string for string in pii_strings if string] + @classmethod def setUpTestData(self): """ @@ -4018,7 +4143,16 @@ def setUpTestData(self): "production", "copyeditor", "typesetter", "proofing-manager", "section-editor"]) - self.regular_user = self.create_user("regularuser@martineve.com") + self.regular_user = self.create_user( + "redshirt@voyager.com", + first_name='Tim', + last_name='Redshirt', + institution='Starfleet Ops', + department='Canon Fodder', + orcid='1234-1234-1234-0000', + country_name='United States of America', + country_code='US', + ) self.regular_user.is_active = True self.regular_user.save() @@ -4040,7 +4174,18 @@ def setUpTestData(self): self.editor.is_active = True self.editor.save() - self.author = self.create_user("authoruser@martineve.com", ["author"], journal=self.journal_one) + self.author = self.create_user( + "b.torres@voyager.com", + ["author"], + journal=self.journal_one, + first_name="Belanna", + last_name="Torres", + institution='Starfleet', + department='Engineering', + orcid='0000-1234-1234-1234', + country_code='FR', + country_name='France', + ) self.author.is_active = True self.author.save() @@ -4107,7 +4252,11 @@ def setUpTestData(self): self.staff_member.is_staff = True self.staff_member.save() - self.repo_manager = self.create_user("repomanager@janeway.systems") + self.repo_manager = self.create_user( + "repomanager@janeway.systems", + first_name='Tom', + last_name='Paris', + ) self.repo_manager.is_active = True self.repo_manager.save() @@ -4150,6 +4299,27 @@ def setUpTestData(self): self.third_file.save() + self.article_in_review = submission_models.Article.objects.create( + owner=self.regular_user, title="A Test Article in review", + abstract="An abstract", + stage=submission_models.STAGE_UNDER_REVIEW, + journal_id=self.journal_one.id, + correspondence_author=self.repo_manager, + ) + self.article_in_review.authors.add( + self.author, + ) + self.article_in_review.snapshot_authors() + review_models.ReviewRound.objects.get_or_create( + article=self.article_in_review, + round_number=1, + ) + review_models.EditorAssignment.objects.get_or_create( + article=self.article_in_review, + editor=self.section_editor, + editor_type='section-editor', + notified=True, + ) self.article_in_production = submission_models.Article(owner=self.regular_user, title="A Test Article", abstract="An abstract", stage=submission_models.STAGE_TYPESETTING, diff --git a/src/submission/forms.py b/src/submission/forms.py index 6778cc00c3..604012de7f 100755 --- a/src/submission/forms.py +++ b/src/submission/forms.py @@ -33,8 +33,12 @@ class ArticleStart(forms.ModelForm): class Meta: model = models.Article - fields = ('publication_fees', 'submission_requirements', 'copyright_notice', - 'competing_interests') + fields = ( + 'publication_fees', + 'submission_requirements', + 'copyright_notice', + 'competing_interests', + ) def __init__(self, *args, **kwargs): journal = kwargs.pop('journal', False) @@ -77,7 +81,7 @@ class Meta: 'language', 'section', 'license', 'primary_issue', 'article_number', 'is_remote', 'remote_url', 'peer_reviewed', 'first_page', 'last_page', 'page_numbers', 'total_pages', - 'competing_interests', 'custom_how_to_cite', 'rights', + 'custom_how_to_cite', 'rights', ) widgets = { 'title': forms.TextInput(attrs={'placeholder': _('Title')}), @@ -208,7 +212,6 @@ def __init__(self, *args, **kwargs): if editor_view: self.fields[element.name].required = False - def save(self, commit=True, request=None): article = super(ArticleInfo, self).save(commit=False) @@ -252,6 +255,11 @@ def __init__(self, *args, **kwargs): "closed for public submission" +class EditArticleMetadata(ArticleInfo): + class Meta(ArticleInfo.Meta): + fields = ArticleInfo.Meta.fields + ('competing_interests',) + + class AuthorForm(forms.ModelForm): class Meta: diff --git a/src/submission/migrations/0080_frozen_author_bleach_20240507_1350.py b/src/submission/migrations/0080_frozen_author_bleach_20240507_1350.py index 2cc7ae6838..d49a0fb616 100644 --- a/src/submission/migrations/0080_frozen_author_bleach_20240507_1350.py +++ b/src/submission/migrations/0080_frozen_author_bleach_20240507_1350.py @@ -2,6 +2,7 @@ import core.model_utils from django.db import migrations, models +import utils.forms class Migration(migrations.Migration): @@ -15,12 +16,12 @@ class Migration(migrations.Migration): migrations.AlterField( model_name='frozenauthor', name='department', - field=core.model_utils.JanewayBleachCharField(blank=True, max_length=300), + field=models.CharField(blank=True, max_length=300, validators=[utils.forms.plain_text_validator]), ), migrations.AlterField( model_name='frozenauthor', name='first_name', - field=core.model_utils.JanewayBleachCharField(blank=True, max_length=300), + field=models.CharField(blank=True, max_length=300, validators=[utils.forms.plain_text_validator]), ), migrations.AlterField( model_name='frozenauthor', @@ -40,26 +41,26 @@ class Migration(migrations.Migration): migrations.AlterField( model_name='frozenauthor', name='institution', - field=core.model_utils.JanewayBleachCharField(blank=True, max_length=1000), + field=models.CharField(blank=True, max_length=1000, validators=[utils.forms.plain_text_validator]), ), migrations.AlterField( model_name='frozenauthor', name='last_name', - field=core.model_utils.JanewayBleachCharField(blank=True, max_length=300), + field=models.CharField(blank=True, max_length=300, validators=[utils.forms.plain_text_validator]), ), migrations.AlterField( model_name='frozenauthor', name='middle_name', - field=core.model_utils.JanewayBleachCharField(blank=True, max_length=300), + field=models.CharField(blank=True, max_length=300, validators=[utils.forms.plain_text_validator]), ), migrations.AlterField( model_name='frozenauthor', name='name_prefix', - field=core.model_utils.JanewayBleachCharField(blank=True, help_text='Optional name prefix (e.g: Prof or Dr)', max_length=300), + field=models.CharField(blank=True, help_text='Optional name prefix (e.g: Prof or Dr)', max_length=300, validators=[utils.forms.plain_text_validator]), ), migrations.AlterField( model_name='frozenauthor', name='name_suffix', - field=core.model_utils.JanewayBleachCharField(blank=True, help_text='Optional name suffix (e.g.: Jr or III)', max_length=300), + field=models.CharField(blank=True, help_text='Optional name suffix (e.g.: Jr or III)', max_length=300, validators=[utils.forms.plain_text_validator]), ), ] diff --git a/src/submission/models.py b/src/submission/models.py index a9b6d80c9b..80e9537765 100755 --- a/src/submission/models.py +++ b/src/submission/models.py @@ -54,6 +54,7 @@ from review import models as review_models from utils.function_cache import cache from utils.logger import get_logger +from utils.forms import plain_text_validator from journal import models as journal_models from review.const import ( ReviewerDecisions as RD, @@ -1911,23 +1912,44 @@ class FrozenAuthor(AbstractLastModifiedModel): on_delete=models.SET_NULL, ) - name_prefix = JanewayBleachCharField( + name_prefix = models.CharField( max_length=300, blank=True, - help_text=_("Optional name prefix (e.g: Prof or Dr)") - - ) - name_suffix = JanewayBleachCharField( + help_text=_("Optional name prefix (e.g: Prof or Dr)"), + validators=[plain_text_validator], + ) + name_suffix = models.CharField( + max_length=300, + blank=True, + help_text=_("Optional name suffix (e.g.: Jr or III)"), + validators=[plain_text_validator], + ) + first_name = models.CharField( max_length=300, blank=True, - help_text=_("Optional name suffix (e.g.: Jr or III)") + validators=[plain_text_validator], + ) + middle_name = models.CharField( + max_length=300, + blank=True, + validators=[plain_text_validator], ) - first_name = JanewayBleachCharField(max_length=300, blank=True) - middle_name = JanewayBleachCharField(max_length=300, blank=True) - last_name = JanewayBleachCharField(max_length=300, blank=True) + last_name = models.CharField( + max_length=300, + blank=True, + validators=[plain_text_validator], +) - institution = JanewayBleachCharField(max_length=1000, blank=True) - department = JanewayBleachCharField(max_length=300, blank=True) + institution = models.CharField( + max_length=1000, + blank=True, + validators=[plain_text_validator], +) + department = models.CharField( + max_length=300, + blank=True, + validators=[plain_text_validator], + ) frozen_biography = JanewayBleachField( blank=True, verbose_name=_('Frozen Biography'), diff --git a/src/submission/tests.py b/src/submission/tests.py index 6b4f57a2ea..de565fe4ba 100644 --- a/src/submission/tests.py +++ b/src/submission/tests.py @@ -13,6 +13,7 @@ from django.utils import translation, timezone from django.urls.base import clear_script_prefix from django.conf import settings +from django.core.exceptions import ValidationError from django.shortcuts import reverse from django.test.utils import override_settings @@ -567,6 +568,33 @@ def test_author_form_with_good_orcid(self): '0000-0003-2126-266X', ) + def test_author_form_harmful_inputs(self): + harmful_string = ' This are not the droids you are looking for ' + for i, attr in enumerate({ + "first_name", + "last_name", + "middle_name", + "name_prefix", + "suffix", + "institution", + "department", + }): + form = forms.AuthorForm( + { + 'first_name': 'Andy', + 'last_name': 'Byers', + 'biography': 'Andy', + 'institution': 'Birkbeck, University of London', + 'email': f'andy{i}@janeway.systems', + 'orcid': 'https://orcid.org/0000-0003-2126-266X', + **{attr: harmful_string}, + } + ) + self.assertFalse( + form.is_valid(), + f"Harmful code injected into field '{attr}'" + ) + @override_settings(URL_CONFIG='domain') def test_article_encoding_bibtex(self): article = helpers.create_article( @@ -906,3 +934,30 @@ def setUpTestData(cls): def test_full_name(self): self.assertEqual('Dr. S. Bella Rogers Esq.', self.frozen_author.full_name()) + + +class ArticleFormTests(TestCase): + + def test_competing_interests_in_edit_article_metadata(self): + form = forms.EditArticleMetadata() + self.assertIn( + 'competing_interests', + form.fields, + "'competing_interests' should be present in EditArticleMetadata", + ) + + def test_competing_interests_not_in_article_info_submit(self): + form = forms.ArticleInfoSubmit() + self.assertNotIn( + 'competing_interests', + form.fields, + "'competing_interests' should NOT be present in ArticleInfoSubmit", + ) + + def test_competing_interests_not_in_editor_article_info_submit(self): + form = forms.EditorArticleInfoSubmit() + self.assertNotIn( + 'competing_interests', + form.fields, + "'competing_interests' should NOT be present in EditorArticleInfoSubmit" + ) diff --git a/src/submission/views.py b/src/submission/views.py index cd2fa76161..dc0afb4f74 100755 --- a/src/submission/views.py +++ b/src/submission/views.py @@ -75,7 +75,13 @@ def start(request, type=None): **{'request': request, 'article': new_article} ) - return redirect(reverse('submit_info', kwargs={'article_id': new_article.pk})) + return redirect( + reverse( + 'submit_info', + kwargs={ + 'article_id': new_article.pk}, + ), + ) template = 'admin/submission/start.html' context = { @@ -697,7 +703,7 @@ def submit_review(request, article_id): return redirect(reverse('core_dashboard')) - template = "admin/submission//submit_review.html" + template = "admin/submission/submit_review.html" context = { 'article': article, 'form': form, @@ -733,7 +739,7 @@ def edit_metadata(request, article_id): article=article, ) - info_form = forms.ArticleInfo( + info_form = forms.EditArticleMetadata( instance=article, additional_fields=additional_fields, submission_summary=submission_summary, @@ -767,7 +773,7 @@ def edit_metadata(request, article_id): return redirect(reverse_url) if 'metadata' in request.POST: - info_form = forms.ArticleInfo( + info_form = forms.EditArticleMetadata( request.POST, instance=article, additional_fields=additional_fields, diff --git a/src/templates/admin/core/dashboard.html b/src/templates/admin/core/dashboard.html index 55ecca7554..d37ad01c73 100644 --- a/src/templates/admin/core/dashboard.html +++ b/src/templates/admin/core/dashboard.html @@ -1,5 +1,5 @@ {% extends "admin/core/base.html" %} -{% load roles %} +{% load roles securitytags %} {% block title %}Dashboard{% endblock title %} {% block title-section %}Dashboard{% endblock %} @@ -192,7 +192,7 @@