From c17d438de8f0da4f158b4630ff317b71749b718d Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 14 Jun 2021 12:40:12 +0100 Subject: [PATCH] DRY-up email revalidation check Previously this was duplicated between the "two_factor" and the "webauthn" views, and required more test setup. This DRYs up the check and tests it once, using mocks to simplify the view tests. As part of DRYing up the check into a util module, I've also moved the "is_less_than_days_ago" function it uses. --- app/main/views/dashboard.py | 2 +- app/main/views/two_factor.py | 8 +++--- app/main/views/webauthn_credentials.py | 9 ++++-- app/models/job.py | 3 +- app/utils/__init__.py | 19 ------------- app/utils/login.py | 5 ++++ app/utils/time.py | 20 +++++++++++++ tests/app/main/views/test_two_factor.py | 27 ++++++++---------- .../main/views/test_webauthn_credentials.py | 7 ++--- tests/app/test_utils.py | 28 +------------------ tests/app/utils/test_login.py | 19 +++++++++++++ tests/app/utils/test_time.py | 25 +++++++++++++++++ 12 files changed, 97 insertions(+), 75 deletions(-) create mode 100644 app/utils/time.py create mode 100644 tests/app/utils/test_login.py create mode 100644 tests/app/utils/test_time.py diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index f5a64d213..a398ef81c 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -30,11 +30,11 @@ from app.utils import ( DELIVERED_STATUSES, FAILURE_STATUSES, REQUESTED_STATUSES, - get_current_financial_year, service_has_permission, ) from app.utils.csv import Spreadsheet from app.utils.pagination import generate_next_dict, generate_previous_dict +from app.utils.time import get_current_financial_year from app.utils.user import user_has_permissions diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py index 75cdd0d78..96b9b68c7 100644 --- a/app/main/views/two_factor.py +++ b/app/main/views/two_factor.py @@ -16,8 +16,8 @@ from app import user_api_client from app.main import main from app.main.forms import TwoFactorForm from app.models.user import User -from app.utils import is_less_than_days_ago from app.utils.login import ( + email_needs_revalidating, log_in_user, redirect_to_sign_in, redirect_when_logged_in, @@ -79,11 +79,11 @@ def two_factor_sms(): redirect_url = request.args.get('next') if form.validate_on_submit(): - if is_less_than_days_ago(user.email_access_validated_at, 90): - return log_in_user(user_id) - else: + if email_needs_revalidating(user): user_api_client.send_verify_code(user.id, 'email', None, redirect_url) return redirect(url_for('.revalidate_email_sent', next=redirect_url)) + else: + return log_in_user(user_id) return render_template('views/two-factor-sms.html', form=form, redirect_url=redirect_url) diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index 008e24ada..2161edf01 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -9,8 +9,11 @@ from app.main import main from app.models.user import User from app.models.webauthn_credential import RegistrationError, WebAuthnCredential from app.notify_client.user_api_client import user_api_client -from app.utils import is_less_than_days_ago -from app.utils.login import log_in_user, redirect_to_sign_in +from app.utils.login import ( + email_needs_revalidating, + log_in_user, + redirect_to_sign_in, +) from app.utils.user import user_is_platform_admin @@ -168,7 +171,7 @@ def _complete_webauthn_login_attempt(user): # user account is locked as too many failed logins abort(403) - if not is_less_than_days_ago(user.email_access_validated_at, 90): + if email_needs_revalidating(user): user_api_client.send_verify_code(user.id, 'email', None, redirect_url) return redirect(url_for('.revalidate_email_sent', next=redirect_url)) diff --git a/app/models/job.py b/app/models/job.py index 89597a5e5..7561ef777 100644 --- a/app/models/job.py +++ b/app/models/job.py @@ -13,8 +13,9 @@ from app.models import JSONModel, ModelList, PaginatedModelList from app.notify_client.job_api_client import job_api_client from app.notify_client.notification_api_client import notification_api_client from app.notify_client.service_api_client import service_api_client -from app.utils import is_less_than_days_ago, set_status_filters +from app.utils import set_status_filters from app.utils.letters import get_letter_printing_statement +from app.utils.time import is_less_than_days_ago class Job(JSONModel): diff --git a/app/utils/__init__.py b/app/utils/__init__.py index 582f3403e..c80aa9b8f 100644 --- a/app/utils/__init__.py +++ b/app/utils/__init__.py @@ -1,14 +1,10 @@ -from datetime import datetime from functools import wraps from itertools import chain from urllib.parse import urlparse -import pytz -from dateutil import parser from flask import abort, current_app, g, make_response, request from flask_login import current_user from notifications_utils.field import Field -from notifications_utils.timezones import utc_string_to_aware_gmt_datetime from orderedset._orderedset import OrderedSet from werkzeug.datastructures import MultiDict from werkzeug.routing import RequestRedirect @@ -40,15 +36,6 @@ def get_help_argument(): return request.args.get('help') if request.args.get('help') in ('1', '2', '3') else None -def get_current_financial_year(): - now = utc_string_to_aware_gmt_datetime( - datetime.utcnow() - ) - current_month = int(now.strftime('%-m')) - current_year = int(now.strftime('%Y')) - return current_year if current_month > 3 else current_year - 1 - - def get_logo_cdn_domain(): parsed_uri = urlparse(current_app.config['ADMIN_BASE_URL']) @@ -114,12 +101,6 @@ class PermanentRedirect(RequestRedirect): code = 301 -def is_less_than_days_ago(date_from_db, number_of_days): - return ( - datetime.utcnow().astimezone(pytz.utc) - parser.parse(date_from_db) - ).days < number_of_days - - def hide_from_search_engines(f): @wraps(f) def decorated_function(*args, **kwargs): diff --git a/app/utils/login.py b/app/utils/login.py index 0c7be1f48..99627c6eb 100644 --- a/app/utils/login.py +++ b/app/utils/login.py @@ -3,6 +3,7 @@ from functools import wraps from flask import redirect, request, session, url_for from app.models.user import User +from app.utils.time import is_less_than_days_ago def redirect_to_sign_in(f): @@ -41,6 +42,10 @@ def redirect_when_logged_in(platform_admin): return redirect(url_for('main.show_accounts_or_dashboard')) +def email_needs_revalidating(user): + return not is_less_than_days_ago(user.email_access_validated_at, 90) + + # see http://flask.pocoo.org/snippets/62/ def _is_safe_redirect_url(target): from urllib.parse import urljoin, urlparse diff --git a/app/utils/time.py b/app/utils/time.py new file mode 100644 index 000000000..efe251814 --- /dev/null +++ b/app/utils/time.py @@ -0,0 +1,20 @@ +from datetime import datetime + +import pytz +from dateutil import parser +from notifications_utils.timezones import utc_string_to_aware_gmt_datetime + + +def get_current_financial_year(): + now = utc_string_to_aware_gmt_datetime( + datetime.utcnow() + ) + current_month = int(now.strftime('%-m')) + current_year = int(now.strftime('%Y')) + return current_year if current_month > 3 else current_year - 1 + + +def is_less_than_days_ago(date_from_db, number_of_days): + return ( + datetime.utcnow().astimezone(pytz.utc) - parser.parse(date_from_db) + ).days < number_of_days diff --git a/tests/app/main/views/test_two_factor.py b/tests/app/main/views/test_two_factor.py index f431a75fd..50c63e4d0 100644 --- a/tests/app/main/views/test_two_factor.py +++ b/tests/app/main/views/test_two_factor.py @@ -1,7 +1,6 @@ import pytest from bs4 import BeautifulSoup from flask import url_for -from freezegun import freeze_time from tests.conftest import ( SERVICE_ONE_ID, @@ -11,6 +10,11 @@ from tests.conftest import ( ) +@pytest.fixture +def mock_email_validated_recently(mocker): + return mocker.patch('app.main.views.two_factor.email_needs_revalidating', return_value=False) + + @pytest.mark.parametrize('request_url', ['two_factor_email_sent', 'revalidate_email_sent']) @pytest.mark.parametrize('redirect_url', [None, f'/services/{SERVICE_ONE_ID}/templates']) @pytest.mark.parametrize('email_resent, page_title', [ @@ -71,7 +75,6 @@ def test_should_render_two_factor_page( )['href'] == url_for('main.check_and_resend_text_code', next=redirect_url) -@freeze_time('2020-01-27T12:00:00') def test_should_login_user_and_should_redirect_to_next_url( client, api_user_active, @@ -79,12 +82,12 @@ def test_should_login_user_and_should_redirect_to_next_url( mock_get_user_by_email, mock_check_verify_code, mock_create_event, + mock_email_validated_recently, ): with client.session_transaction() as session: session['user_details'] = { 'id': api_user_active['id'], 'email': api_user_active['email_address']} - api_user_active['email_access_validated_at'] = '2020-01-23T11:35:21.726132Z' response = client.post(url_for('main.two_factor_sms', next='/services/{}'.format(SERVICE_ONE_ID)), data={'sms_code': '12345'}) @@ -96,7 +99,6 @@ def test_should_login_user_and_should_redirect_to_next_url( ) -@freeze_time('2020-01-27T12:00:00') def test_should_send_email_and_redirect_to_info_page_if_user_needs_to_revalidate_email( client, api_user_active, @@ -107,7 +109,7 @@ def test_should_send_email_and_redirect_to_info_page_if_user_needs_to_revalidate mocker ): mocker.patch('app.user_api_client.get_user', return_value=api_user_active) - api_user_active['email_access_validated_at'] = '2019-03-23T11:35:21.726132Z' + mocker.patch('app.main.views.two_factor.email_needs_revalidating', return_value=True) with client.session_transaction() as session: session['user_details'] = { 'id': api_user_active['id'], @@ -124,7 +126,6 @@ def test_should_send_email_and_redirect_to_info_page_if_user_needs_to_revalidate mock_send_verify_code.assert_called_with(api_user_active['id'], 'email', None, mocker.ANY) -@freeze_time('2020-01-27T12:00:00') def test_should_login_user_and_not_redirect_to_external_url( client, api_user_active, @@ -133,12 +134,12 @@ def test_should_login_user_and_not_redirect_to_external_url( mock_check_verify_code, mock_get_services_with_one_service, mock_create_event, + mock_email_validated_recently, ): with client.session_transaction() as session: session['user_details'] = { 'id': api_user_active['id'], 'email': api_user_active['email_address']} - api_user_active['email_access_validated_at'] = '2020-01-23T11:35:21.726132Z' response = client.post(url_for('main.two_factor_sms', next='http://www.google.com'), data={'sms_code': '12345'}) @@ -149,7 +150,6 @@ def test_should_login_user_and_not_redirect_to_external_url( @pytest.mark.parametrize('platform_admin', ( True, False, )) -@freeze_time('2020-01-27T12:00:00') def test_should_login_user_and_redirect_to_show_accounts( client, api_user_active, @@ -157,13 +157,13 @@ def test_should_login_user_and_redirect_to_show_accounts( mock_get_user_by_email, mock_check_verify_code, mock_create_event, + mock_email_validated_recently, platform_admin, ): with client.session_transaction() as session: session['user_details'] = { 'id': api_user_active['id'], 'email': api_user_active['email_address']} - api_user_active['email_access_validated_at'] = '2020-01-23T11:35:21.726132Z' api_user_active['platform_admin'] = platform_admin response = client.post(url_for('main.two_factor_sms'), @@ -192,7 +192,6 @@ def test_should_return_200_with_sms_code_error_when_sms_code_is_wrong( assert 'Code not found' in response.get_data(as_text=True) -@freeze_time('2020-01-27T12:00:00') def test_should_login_user_when_multiple_valid_codes_exist( client, api_user_active, @@ -201,19 +200,18 @@ def test_should_login_user_when_multiple_valid_codes_exist( mock_check_verify_code, mock_get_services_with_one_service, mock_create_event, + mock_email_validated_recently, ): with client.session_transaction() as session: session['user_details'] = { 'id': api_user_active['id'], 'email': api_user_active['email_address']} - api_user_active['email_access_validated_at'] = '2020-01-23T11:35:21.726132Z' response = client.post(url_for('main.two_factor_sms'), data={'sms_code': '23456'}) assert response.status_code == 302 -@freeze_time('2020-01-27T12:00:00') def test_two_factor_should_set_password_when_new_password_exists_in_session( client, api_user_active, @@ -222,13 +220,13 @@ def test_two_factor_should_set_password_when_new_password_exists_in_session( mock_get_services_with_one_service, mock_update_user_password, mock_create_event, + mock_email_validated_recently, ): with client.session_transaction() as session: session['user_details'] = { 'id': api_user_active['id'], 'email': api_user_active['email_address'], 'password': 'changedpassword'} - api_user_active['email_access_validated_at'] = '2020-01-23T11:35:21.726132Z' response = client.post(url_for('main.two_factor_sms'), data={'sms_code': '12345'}) @@ -279,7 +277,6 @@ def test_two_factor_get_should_redirect_to_sign_in_if_user_not_in_session( ) -@freeze_time('2020-01-27T12:00:00') def test_two_factor_should_activate_pending_user( client, mocker, @@ -287,10 +284,10 @@ def test_two_factor_should_activate_pending_user( mock_check_verify_code, mock_create_event, mock_activate_user, + mock_email_validated_recently, ): mocker.patch('app.user_api_client.get_user', return_value=api_user_pending) mocker.patch('app.service_api_client.get_services', return_value={'data': []}) - api_user_pending['email_access_validated_at'] = '2020-01-23T11:35:21.726132Z' with client.session_transaction() as session: session['user_details'] = { 'id': api_user_pending['id'], diff --git a/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index f7afdddf2..1b4ba5f31 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/tests/app/main/views/test_webauthn_credentials.py @@ -4,7 +4,6 @@ from unittest.mock import ANY, Mock import pytest from fido2 import cbor from flask import url_for -from freezegun.api import freeze_time from app.models.webauthn_credential import RegistrationError, WebAuthnCredential @@ -336,7 +335,6 @@ def test_complete_authentication_clears_session( assert 'webauthn_authentication_state' not in session -@freeze_time('2020-01-30') @pytest.mark.parametrize('url_kwargs, expected_redirect', [ ({}, '/accounts-or-dashboard'), ({'next': '/bar'}, '/bar'), @@ -350,7 +348,6 @@ def test_verify_webauthn_login_signs_user_in( expected_redirect, ): platform_admin_user['auth_type'] = 'webauthn_auth' - platform_admin_user['email_access_validated_at'] = '2020-01-25T00:00:00.000000Z' with client.session_transaction() as session: session['user_details'] = { @@ -360,6 +357,7 @@ def test_verify_webauthn_login_signs_user_in( mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_authentication') mocker.patch('app.user_api_client.complete_webauthn_login_attempt', return_value=(True, None)) + mocker.patch('app.main.views.webauthn_credentials.email_needs_revalidating', return_value=False) resp = client.post(url_for('main.webauthn_complete_authentication', **url_kwargs)) @@ -397,7 +395,6 @@ def test_verify_webauthn_login_signs_user_in_doesnt_sign_user_in_if_api_rejects( assert resp.status_code == 403 -@freeze_time('2020-04-30') def test_verify_webauthn_login_signs_user_in_sends_revalidation_email_if_needed( client, mocker, @@ -405,7 +402,6 @@ def test_verify_webauthn_login_signs_user_in_sends_revalidation_email_if_needed( platform_admin_user, ): platform_admin_user['auth_type'] = 'webauthn_auth' - platform_admin_user['email_access_validated_at'] = '2020-01-25T00:00:00.000000Z' user_details = { 'id': platform_admin_user['id'], 'email': platform_admin_user['email_address'] @@ -417,6 +413,7 @@ def test_verify_webauthn_login_signs_user_in_sends_revalidation_email_if_needed( mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_authentication') mocker.patch('app.user_api_client.complete_webauthn_login_attempt', return_value=(True, None)) + mocker.patch('app.main.views.webauthn_credentials.email_needs_revalidating', return_value=True) resp = client.post(url_for('main.webauthn_complete_authentication')) diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 68536b22e..9503a2f51 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -3,12 +3,7 @@ from freezegun import freeze_time from app import format_datetime_relative from app.formatters import email_safe, round_to_significant_figures -from app.utils import ( - get_current_financial_year, - get_logo_cdn_domain, - is_less_than_days_ago, - merge_jsonlike, -) +from app.utils import get_logo_cdn_domain, merge_jsonlike @pytest.mark.parametrize('service_name, safe_email', [ @@ -79,16 +74,6 @@ def test_format_datetime_relative(time, human_readable_datetime): assert format_datetime_relative(time) == human_readable_datetime -@pytest.mark.parametrize("date_from_db, expected_result", [ - ('2019-11-17T11:35:21.726132Z', True), - ('2019-11-16T11:35:21.726132Z', False), - ('2019-11-16T11:35:21+0000', False), -]) -@freeze_time('2020-02-14T12:00:00') -def test_is_less_than_days_ago(date_from_db, expected_result): - assert is_less_than_days_ago(date_from_db, 90) == expected_result - - @pytest.mark.parametrize("source_object, destination_object, expected_result", [ # simple dicts: ({"a": "b"}, {"c": "d"}, {"a": "b", "c": "d"}), @@ -137,14 +122,3 @@ def test_merge_jsonlike_merges_jsonlike_objects_correctly(source_object, destina )) def test_round_to_significant_figures(value, significant_figures, expected_result): assert round_to_significant_figures(value, significant_figures) == expected_result - - -@pytest.mark.parametrize('datetime_string, financial_year', ( - ('2021-01-01T00:00:00+00:00', 2020), # Start of 2021 - ('2021-03-31T22:59:59+00:00', 2020), # One minute before midnight (BST) - ('2021-03-31T23:00:00+00:00', 2021), # Midnight (BST) - ('2021-12-12T12:12:12+01:00', 2021), # Later in the year -)) -def test_get_financial_year(datetime_string, financial_year): - with freeze_time(datetime_string): - assert get_current_financial_year() == financial_year diff --git a/tests/app/utils/test_login.py b/tests/app/utils/test_login.py new file mode 100644 index 000000000..c376561eb --- /dev/null +++ b/tests/app/utils/test_login.py @@ -0,0 +1,19 @@ +import pytest +from freezegun import freeze_time + +from app.models.user import User +from app.utils.login import email_needs_revalidating + + +@freeze_time('2020-11-27T12:00:00') +@pytest.mark.parametrize(('email_access_validated_at', 'expected_result'), ( + ('2020-10-01T11:35:21.726132Z', False), + ('2020-07-23T11:35:21.726132Z', True), +)) +def test_email_needs_revalidating( + api_user_active, + email_access_validated_at, + expected_result, +): + api_user_active['email_access_validated_at'] = email_access_validated_at + assert email_needs_revalidating(User(api_user_active)) == expected_result diff --git a/tests/app/utils/test_time.py b/tests/app/utils/test_time.py new file mode 100644 index 000000000..509154c41 --- /dev/null +++ b/tests/app/utils/test_time.py @@ -0,0 +1,25 @@ +import pytest +from freezegun import freeze_time + +from app.utils.time import get_current_financial_year, is_less_than_days_ago + + +@pytest.mark.parametrize("date_from_db, expected_result", [ + ('2019-11-17T11:35:21.726132Z', True), + ('2019-11-16T11:35:21.726132Z', False), + ('2019-11-16T11:35:21+0000', False), +]) +@freeze_time('2020-02-14T12:00:00') +def test_is_less_than_days_ago(date_from_db, expected_result): + assert is_less_than_days_ago(date_from_db, 90) == expected_result + + +@pytest.mark.parametrize('datetime_string, financial_year', ( + ('2021-01-01T00:00:00+00:00', 2020), # Start of 2021 + ('2021-03-31T22:59:59+00:00', 2020), # One minute before midnight (BST) + ('2021-03-31T23:00:00+00:00', 2021), # Midnight (BST) + ('2021-12-12T12:12:12+01:00', 2021), # Later in the year +)) +def test_get_financial_year(datetime_string, financial_year): + with freeze_time(datetime_string): + assert get_current_financial_year() == financial_year