From cb5941358111f8ff257b75306007f6b7e68b891b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 17 Aug 2021 16:14:47 +0100 Subject: [PATCH] Update `email_access_validated_at` on link click MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When someone uses a fresh password reset link they have proved that they have access to their inbox. At the moment, when revalidating a user’s email address we wait until after they’ve put in the 2FA code before updating the timestamp which records when they last validated their email address[1]. We can’t think of a good reason that we need the extra assurance of a valid 2FA code to assert that the user has access to their email – they’ve done that just by clicking the link. When the user clicks the link we already update their failed login count before they 2fa. Think it makes sense to handle `email_access_validated_at` then too. As a bonus, the functional tests never go as far as getting a 2FA code after a password reset[2], so the functional test user never gets its timestamp updated. This causes the functional tests start failing after 90 days. By moving the update to this point we ensure that the functional tests will keep passing indefinitely. 1. This code in the API (https://github.com/alphagov/notifications-api/blob/91542ad33eb995c9b3e617cde30a9714846dd37b/app/dao/users_dao.py#L131) which is called by this code in the admin app (https://github.com/alphagov/notifications-admin/blob/9ba37249a47f08b1396e63a34bc591c781739686/app/utils/login.py#L26) 2. https://github.com/alphagov/notifications-functional-tests/blob/5837eb01dc5c9b5bb255d2d1c54e64f778ff4f4e/tests/functional/preview_and_dev/test_email_auth.py#L43-L46 --- app/main/views/new_password.py | 3 +++ app/models/user.py | 7 +++++++ app/notify_client/user_api_client.py | 3 ++- tests/app/main/views/test_new_password.py | 18 +++++++++++++++--- 4 files changed, 27 insertions(+), 4 deletions(-) diff --git a/app/main/views/new_password.py b/app/main/views/new_password.py index 210496447..b44d1e8b1 100644 --- a/app/main/views/new_password.py +++ b/app/main/views/new_password.py @@ -33,6 +33,9 @@ def new_password(token): flash('The link in the email has already been used') return redirect(url_for('main.index')) + if request.method == 'GET': + user.update_email_access_validated_at() + form = NewPasswordForm() if form.validate_on_submit(): diff --git a/app/models/user.py b/app/models/user.py index 95278abb6..690a2d092 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -1,3 +1,5 @@ +from datetime import datetime + from flask import abort, request, session from flask_login import AnonymousUserMixin, UserMixin, login_user, logout_user from notifications_python_client.errors import HTTPError @@ -116,6 +118,11 @@ class User(JSONModel, UserMixin): response = user_api_client.update_password(self.id, password, validated_email_access=validated_email_access) self.__init__(response) + def update_email_access_validated_at(self): + self.update( + email_access_validated_at=datetime.utcnow().isoformat() + ) + def password_changed_more_recently_than(self, datetime_string): if not self.password_changed_at: return False diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 7360e2209..ef69e1380 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -9,7 +9,8 @@ ALLOWED_ATTRIBUTES = { 'mobile_number', 'auth_type', 'updated_by', - 'current_session_id' + 'current_session_id', + 'email_access_validated_at', } diff --git a/tests/app/main/views/test_new_password.py b/tests/app/main/views/test_new_password.py index b0c690d1f..b726d124f 100644 --- a/tests/app/main/views/test_new_password.py +++ b/tests/app/main/views/test_new_password.py @@ -3,21 +3,28 @@ from datetime import datetime import pytest from flask import url_for +from freezegun import freeze_time from itsdangerous import SignatureExpired from notifications_utils.url_safe_token import generate_token from tests.conftest import SERVICE_ONE_ID, url_for_endpoint_with_token +@freeze_time("2021-01-01 11:11:11") def test_should_render_new_password_template( + mocker, notify_admin, client, - api_user_active, - mock_login, mock_send_verify_code, mock_get_user_by_email_request_password_reset, ): - data = json.dumps({'email': api_user_active['email_address'], 'created_at': str(datetime.utcnow())}) + user = mock_get_user_by_email_request_password_reset.return_value + user['password_changed_at'] = '2021-01-01 00:00:00' + mock_update_user_attribute = mocker.patch( + 'app.user_api_client.update_user_attribute', + return_value=user, + ) + data = json.dumps({'email': user['email_address'], 'created_at': str(datetime.utcnow())}) token = generate_token(data, notify_admin.config['SECRET_KEY'], notify_admin.config['DANGEROUS_SALT']) @@ -25,6 +32,11 @@ def test_should_render_new_password_template( assert response.status_code == 200 assert 'You can now create a new password for your account.' in response.get_data(as_text=True) + mock_update_user_attribute.assert_called_once_with( + user['id'], + email_access_validated_at='2021-01-01T11:11:11' + ) + def test_should_return_404_when_email_address_does_not_exist( notify_admin,