From b15d3434c300a681384d68ccd2fd6de14ca205e3 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 7 Mar 2016 14:34:53 +0000 Subject: [PATCH 1/8] Added an endpoint and celery task to email a reset password url. --- app/celery/tasks.py | 12 ++++++++ app/user/rest.py | 34 ++++++++++++++++++--- config.py | 1 + tests/app/celery/test_tasks.py | 25 ++++++++++++++- tests/app/user/test_rest.py | 49 ++++++++++++++++++++++++++++-- tests/app/user/test_rest_verify.py | 20 ++---------- 6 files changed, 115 insertions(+), 26 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index d4388a08c..c1306193a 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -251,3 +251,15 @@ def email_invited_user(encrypted_invitation): invitation_content) except AwsSesClientException as e: current_app.logger.error(e) + + +@notify_celery.task(name='send-reset-password') +def email_reset_password(encrypted_reset_password_message): + reset_password_message = encryption.decrypt(encryption) + try: + aws_ses_client.send_email(current_app.config['VERIFY_CODE_FROM_EMAIL_ADDRESS'], + reset_password_message['to'], + "Reset password for GOV.UK Notify", + reset_password_message['reset_password_url']) + except AwsSesClientException as e: + current_app.logger.error(e) diff --git a/app/user/rest.py b/app/user/rest.py index 9977410db..716eea2be 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -24,7 +24,7 @@ from app.schemas import ( permission_schema ) -from app.celery.tasks import (send_sms_code, send_email_code) +from app.celery.tasks import (send_sms_code, send_email_code, email_reset_password) from app.errors import register_errors user = Blueprint('user', __name__) @@ -49,7 +49,7 @@ def create_user(): def update_user(user_id): user_to_update = get_model_users(user_id=user_id) if not user_to_update: - return jsonify(result="error", message="User not found"), 404 + return _user_not_found(user_id) req_json = request.get_json() update_dct, errors = user_schema_load_json.load(req_json) @@ -116,7 +116,7 @@ def send_user_sms_code(user_id): user_to_send_to = get_model_users(user_id=user_id) if not user_to_send_to: - return jsonify(result="error", message="No user found"), 404 + return _user_not_found(user_id) verify_code, errors = request_verify_code_schema.load(request.get_json()) if errors: @@ -138,7 +138,7 @@ def send_user_sms_code(user_id): def send_user_email_code(user_id): user_to_send_to = get_model_users(user_id=user_id) if not user_to_send_to: - return jsonify(result="error", message="No user found"), 404 + return _user_not_found(user_id) verify_code, errors = request_verify_code_schema.load(request.get_json()) if errors: @@ -172,7 +172,7 @@ def set_permissions(user_id, service_id): # who is making this request has permission to make the request. user = get_model_users(user_id=user_id) if not user: - abort(404, 'User not found for id: {}'.format(user_id)) + _user_not_found(user_id) service = dao_fetch_service_by_id(service_id=service_id) if not service: abort(404, 'Service not found for id: {}'.format(service_id)) @@ -197,3 +197,27 @@ def get_by_email(): result = user_schema.dump(user) return jsonify(data=result.data) + + +@user.route('//reset-password', methods=['POST']) +def send_reset_password(user_id): + user_to_send_to = get_model_users(user_id=user_id) + if not user_to_send_to: + return _user_not_found(user_id) + + reset_password_message = {'to': user_to_send_to.email_address, + 'reset_password_url': _create_reset_password_url(user_to_send_to.email_address)} + + email_reset_password.apply_async([encryption.encrypt(reset_password_message)], queue='send-reset-password') + return jsonify({}), 204 + + +def _user_not_found(user_id): + return abort(404, 'User not found for id: {}'.format(user_id)) + + +def _create_reset_password_url(email): + from utils.url_safe_token import generate_token + token = generate_token(email, current_app.config['SECRET_KEY'], current_app.config['DANGEROUS_SALT']) + + return current_app.config['ADMIN_BASE_URL'] + '/new-password/' + token diff --git a/config.py b/config.py index 7237a2c07..dd6fa5652 100644 --- a/config.py +++ b/config.py @@ -48,6 +48,7 @@ class Config(object): Queue('email', Exchange('default'), routing_key='email'), Queue('sms-code', Exchange('default'), routing_key='sms-code'), Queue('email-code', Exchange('default'), routing_key='email-code'), + Queue('email-forgot-password', Exchange('default'), routing_key='email-forgot-password'), Queue('process-job', Exchange('default'), routing_key='process-job'), Queue('bulk-sms', Exchange('default'), routing_key='bulk-sms'), Queue('bulk-email', Exchange('default'), routing_key='bulk-email'), diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 6ee7c5391..0e2acef20 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,7 +1,13 @@ import uuid import pytest from flask import current_app -from app.celery.tasks import (send_sms, send_sms_code, send_email_code, send_email, process_job, email_invited_user) +from app.celery.tasks import (send_sms, + send_sms_code, + send_email_code, + send_email, + process_job, + email_invited_user, + email_reset_password) from app import (firetext_client, aws_ses_client, encryption) from app.clients.email.aws_ses import AwsSesClientException from app.clients.sms.firetext import FiretextClientException @@ -503,3 +509,20 @@ def test_email_invited_user_should_send_email(notify_api, mocker): invitation['to'], expected_subject, expected_content) + + +def test_email_reset_password_should_send_email(notify_api, mocker): + with notify_api.test_request_context(): + reset_password_message = {'to': 'someone@it.gov.uk', + 'reset_password_url': 'bah'} + + mocker.patch('app.aws_ses_client.send_email') + mocker.patch('app.encryption.decrypt', return_value=reset_password_message) + + encrypted_message = encryption.encrypt(reset_password_message) + email_reset_password(encrypted_message) + + aws_ses_client.send_email(current_app.config['VERIFY_CODE_FROM_EMAIL_ADDRESS'], + reset_password_message['to'], + "Reset password for GOV.UK Notify", + reset_password_message['reset_password_url']) diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index c014272a1..f1aa634a7 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -1,10 +1,12 @@ import json +import uuid from flask import url_for +import app from app.models import (User, Permission, MANAGE_SETTINGS, MANAGE_TEMPLATES) from app.dao.permissions_dao import default_service_permissions -from app import db +from app import db, encryption from tests import create_authorization_header @@ -256,7 +258,7 @@ def test_put_user_not_exists(notify_api, notify_db, notify_db_session, sample_us user = User.query.filter_by(id=sample_user.id).first() json_resp = json.loads(resp.get_data(as_text=True)) assert json_resp['result'] == "error" - assert json_resp['message'] == "User not found" + assert json_resp['message'] == "User not found for id: {}".format("9999") assert user == sample_user assert user.email_address != new_email @@ -426,3 +428,46 @@ def test_set_user_permissions_remove_old(notify_api, query = Permission.query.filter_by(user=sample_user) assert query.count() == 1 assert query.first().permission == MANAGE_SETTINGS + + +def test_send_reset_password_should_send_reset_password_link(notify_api, + sample_user, + mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.email_reset_password.apply_async') + auth_header = create_authorization_header( + path=url_for('user.send_reset_password', user_id=sample_user.id), + method='POST', + request_body={}) + resp = client.post( + url_for('user.send_reset_password', user_id=sample_user.id), + data={}, + headers=[('Content-Type', 'application/json'), auth_header]) + + assert resp.status_code == 204 + from app.user.rest import _create_reset_password_url + url = _create_reset_password_url(sample_user.email_address) + encrypted = encryption.encrypt({'to': sample_user.email_address, 'reset_password_url': url}) + app.celery.tasks.email_reset_password.apply_async.assert_called_once_with([encrypted], + queue='send-reset-password') + + +def test_send_reset_password_should_return_404_when_user_doesnot_exist(notify_api, + sample_user, + mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + user_id = 99999 + auth_header = create_authorization_header( + path=url_for('user.send_reset_password', user_id=user_id), + method='POST', + request_body={}) + + resp = client.post( + url_for('user.send_reset_password', user_id=user_id), + data={}, + headers=[('Content-Type', 'application/json'), auth_header]) + + assert resp.status_code == 404 + assert json.loads(resp.get_data(as_text=True))['message'] == 'User not found for id: {}'.format(user_id) diff --git a/tests/app/user/test_rest_verify.py b/tests/app/user/test_rest_verify.py index 10310627c..a337ffa5e 100644 --- a/tests/app/user/test_rest_verify.py +++ b/tests/app/user/test_rest_verify.py @@ -9,8 +9,6 @@ from tests import create_authorization_header def test_user_verify_code_sms(notify_api, - notify_db, - notify_db_session, sample_sms_code): """ Tests POST endpoint '//verify/code' @@ -34,8 +32,6 @@ def test_user_verify_code_sms(notify_api, def test_user_verify_code_sms_missing_code(notify_api, - notify_db, - notify_db_session, sample_sms_code): """ Tests POST endpoint '//verify/code' @@ -58,8 +54,6 @@ def test_user_verify_code_sms_missing_code(notify_api, @moto.mock_sqs def test_user_verify_code_email(notify_api, - notify_db, - notify_db_session, sqs_client_conn, sample_email_code): """ @@ -84,8 +78,6 @@ def test_user_verify_code_email(notify_api, def test_user_verify_code_email_bad_code(notify_api, - notify_db, - notify_db_session, sample_email_code): """ Tests POST endpoint '//verify/code' @@ -109,8 +101,6 @@ def test_user_verify_code_email_bad_code(notify_api, def test_user_verify_code_email_expired_code(notify_api, - notify_db, - notify_db_session, sample_email_code): """ Tests POST endpoint '//verify/code' @@ -159,8 +149,6 @@ def test_user_verify_password(notify_api, def test_user_verify_password_invalid_password(notify_api, - notify_db, - notify_db_session, sample_user): """ Tests POST endpoint '//verify/password' invalid endpoint. @@ -186,8 +174,6 @@ def test_user_verify_password_invalid_password(notify_api, def test_user_verify_password_valid_password_resets_failed_logins(notify_api, - notify_db, - notify_db_session, sample_user): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -224,8 +210,6 @@ def test_user_verify_password_valid_password_resets_failed_logins(notify_api, def test_user_verify_password_missing_password(notify_api, - notify_db, - notify_db_session, sample_user): """ Tests POST endpoint '//verify/password' missing password. @@ -311,7 +295,7 @@ def test_send_sms_code_returns_404_for_bad_input_data(notify_api, notify_db, not data=data, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 404 - assert json.loads(resp.get_data(as_text=True))['message'] == 'No user found' + assert json.loads(resp.get_data(as_text=True))['message'] == 'User not found for id: {}'.format(int(uuid_)) def test_send_user_email_code(notify_api, @@ -353,4 +337,4 @@ def test_send_user_email_code_returns_404_for_when_user_does_not_exist(notify_ap data=data, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 404 - assert json.loads(resp.get_data(as_text=True))['message'] == 'No user found' + assert json.loads(resp.get_data(as_text=True))['message'] == 'User not found for id: {}'.format(1) From 10296f0cc213b42a9799505d0725f918a17ea67e Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 7 Mar 2016 15:21:05 +0000 Subject: [PATCH 2/8] Send email address in the data rather than the user_id as a path param. Remove unused OldRequestVerifyCodeSchema. --- app/schemas.py | 23 +++++++---------- app/user/rest.py | 26 +++++++++++++------- tests/app/user/test_rest.py | 49 ++++++++++++++++++++++++++----------- 3 files changed, 61 insertions(+), 37 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index 40cb7ff08..2673f999a 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -1,5 +1,4 @@ import re -from flask import current_app from flask_marshmallow.fields import fields from . import ma from . import models @@ -102,18 +101,6 @@ class JobSchema(BaseSchema): model = models.Job -# TODO: Remove this schema once the admin app has stopped using the /user/code endpoint -class OldRequestVerifyCodeSchema(ma.Schema): - - code_type = fields.Str(required=True) - to = fields.Str(required=False) - - @validates('code_type') - def validate_code_type(self, code): - if code not in models.VERIFY_CODE_TYPES: - raise ValidationError('Invalid code type') - - class RequestVerifyCodeSchema(ma.Schema): to = fields.Str(required=False) @@ -195,6 +182,14 @@ class PermissionSchema(BaseSchema): exclude = ("created_at",) +class EmailDataSchema(ma.Schema): + email = fields.Str(required=False) + + @validates('email') + def validate_to(self, value): + if not email_regex.match(value): + raise ValidationError('Invalid email') + user_schema = UserSchema() user_schema_load_json = UserSchema(load_json=True) service_schema = ServiceSchema() @@ -205,7 +200,6 @@ api_key_schema = ApiKeySchema() api_key_schema_load_json = ApiKeySchema(load_json=True) job_schema = JobSchema() job_schema_load_json = JobSchema(load_json=True) -old_request_verify_code_schema = OldRequestVerifyCodeSchema() request_verify_code_schema = RequestVerifyCodeSchema() sms_admin_notification_schema = SmsAdminNotificationSchema() sms_template_notification_schema = SmsTemplateNotificationSchema() @@ -216,3 +210,4 @@ notification_status_schema = NotificationStatusSchema() notification_status_schema_load_json = NotificationStatusSchema(load_json=True) invited_user_schema = InvitedUserSchema() permission_schema = PermissionSchema() +email_data_request_schema = EmailDataSchema() diff --git a/app/user/rest.py b/app/user/rest.py index 716eea2be..3e3e1ec00 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -17,7 +17,7 @@ from app.dao.permissions_dao import permission_dao from app.dao.services_dao import dao_fetch_service_by_id from app.schemas import ( - old_request_verify_code_schema, + email_data_request_schema, user_schema, request_verify_code_schema, user_schema_load_json, @@ -191,19 +191,23 @@ def get_by_email(): email = request.args.get('email') if not email: return jsonify(result="error", message="invalid request"), 400 - user = get_user_by_email(email) - if not user: - return jsonify(result="error", message="not found"), 404 - result = user_schema.dump(user) + fetched_user = get_user_by_email(email) + if not fetched_user: + return _user_not_found_for_email(email) + result = user_schema.dump(fetched_user) return jsonify(data=result.data) -@user.route('//reset-password', methods=['POST']) -def send_reset_password(user_id): - user_to_send_to = get_model_users(user_id=user_id) +@user.route('/reset-password', methods=['POST']) +def send_reset_password(): + email, errors = email_data_request_schema.load(request.get_json()) + if errors: + return jsonify(result="error", message=errors), 400 + + user_to_send_to = get_user_by_email(email['email']) if not user_to_send_to: - return _user_not_found(user_id) + return _user_not_found_for_email(email['email']) reset_password_message = {'to': user_to_send_to.email_address, 'reset_password_url': _create_reset_password_url(user_to_send_to.email_address)} @@ -216,6 +220,10 @@ def _user_not_found(user_id): return abort(404, 'User not found for id: {}'.format(user_id)) +def _user_not_found_for_email(email): + return abort(404, 'User not found for email address: {}'.format(email)) + + def _create_reset_password_url(email): from utils.url_safe_token import generate_token token = generate_token(email, current_app.config['SECRET_KEY'], current_app.config['DANGEROUS_SALT']) diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index f1aa634a7..74af49d5a 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -286,7 +286,7 @@ def test_get_user_by_email(notify_api, notify_db, notify_db_session, sample_serv assert sorted(expected_permissions) == sorted(fetched['permissions'][str(sample_service.id)]) -def test_get_user_by_email_not_found_returns_400(notify_api, +def test_get_user_by_email_not_found_returns_404(notify_api, notify_db, notify_db_session, sample_user): @@ -299,7 +299,7 @@ def test_get_user_by_email_not_found_returns_400(notify_api, assert resp.status_code == 404 json_resp = json.loads(resp.get_data(as_text=True)) assert json_resp['result'] == 'error' - assert json_resp['message'] == 'not found' + assert json_resp['message'] == 'User not found for email address: {}'.format('no_user@digital.gov.uk') def test_get_user_by_email_bad_url_returns_404(notify_api, @@ -436,13 +436,14 @@ def test_send_reset_password_should_send_reset_password_link(notify_api, with notify_api.test_request_context(): with notify_api.test_client() as client: mocker.patch('app.celery.tasks.email_reset_password.apply_async') + data = json.dumps({'email': sample_user.email_address}) auth_header = create_authorization_header( - path=url_for('user.send_reset_password', user_id=sample_user.id), + path=url_for('user.send_reset_password'), method='POST', - request_body={}) + request_body=data) resp = client.post( - url_for('user.send_reset_password', user_id=sample_user.id), - data={}, + url_for('user.send_reset_password'), + data=data, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 204 @@ -453,21 +454,41 @@ def test_send_reset_password_should_send_reset_password_link(notify_api, queue='send-reset-password') -def test_send_reset_password_should_return_404_when_user_doesnot_exist(notify_api, - sample_user, +def test_send_reset_password_should_return_400_when_user_doesnot_exist(notify_api, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: - user_id = 99999 + bad_email_address = 'bad@email.gov.uk' + data = json.dumps({'email': bad_email_address}) auth_header = create_authorization_header( - path=url_for('user.send_reset_password', user_id=user_id), + path=url_for('user.send_reset_password'), method='POST', - request_body={}) + request_body=data) resp = client.post( - url_for('user.send_reset_password', user_id=user_id), - data={}, + url_for('user.send_reset_password'), + data=data, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 404 - assert json.loads(resp.get_data(as_text=True))['message'] == 'User not found for id: {}'.format(user_id) + assert json.loads(resp.get_data(as_text=True))['message'] == 'User not found for email address: {}'.format( + bad_email_address) + + +def test_send_reset_password_should_return_400_when_data_is_not_email_address(notify_api, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + bad_email_address = 'bad.email.gov.uk' + data = json.dumps({'email': bad_email_address}) + auth_header = create_authorization_header( + path=url_for('user.send_reset_password'), + method='POST', + request_body=data) + + resp = client.post( + url_for('user.send_reset_password'), + data=data, + headers=[('Content-Type', 'application/json'), auth_header]) + + assert resp.status_code == 400 + assert json.loads(resp.get_data(as_text=True))['message'] == {'email': ['Invalid email']} From 5c4ac9d93849bcf1d9ea8440f1ad2d8d8d3a355e Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 7 Mar 2016 18:20:20 +0000 Subject: [PATCH 3/8] Include token creation date in the url token. --- app/celery/tasks.py | 4 ++-- app/user/rest.py | 17 ++++++++++------- config.py | 2 +- tests/app/user/test_rest.py | 37 +++++++++++++++++-------------------- 4 files changed, 30 insertions(+), 30 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index c1306193a..968058fd0 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -253,9 +253,9 @@ def email_invited_user(encrypted_invitation): current_app.logger.error(e) -@notify_celery.task(name='send-reset-password') +@notify_celery.task(name='email-reset-password') def email_reset_password(encrypted_reset_password_message): - reset_password_message = encryption.decrypt(encryption) + reset_password_message = encryption.decrypt(encrypted_reset_password_message) try: aws_ses_client.send_email(current_app.config['VERIFY_CODE_FROM_EMAIL_ADDRESS'], reset_password_message['to'], diff --git a/app/user/rest.py b/app/user/rest.py index 3e3e1ec00..fbedd452d 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -193,26 +193,27 @@ def get_by_email(): return jsonify(result="error", message="invalid request"), 400 fetched_user = get_user_by_email(email) if not fetched_user: - return _user_not_found_for_email(email) + return _user_not_found_for_email() result = user_schema.dump(fetched_user) return jsonify(data=result.data) @user.route('/reset-password', methods=['POST']) -def send_reset_password(): +def send_user_reset_password(): email, errors = email_data_request_schema.load(request.get_json()) if errors: return jsonify(result="error", message=errors), 400 user_to_send_to = get_user_by_email(email['email']) if not user_to_send_to: - return _user_not_found_for_email(email['email']) + return _user_not_found_for_email() reset_password_message = {'to': user_to_send_to.email_address, 'reset_password_url': _create_reset_password_url(user_to_send_to.email_address)} - email_reset_password.apply_async([encryption.encrypt(reset_password_message)], queue='send-reset-password') + email_reset_password.apply_async([encryption.encrypt(reset_password_message)], queue='email-reset-password') + return jsonify({}), 204 @@ -220,12 +221,14 @@ def _user_not_found(user_id): return abort(404, 'User not found for id: {}'.format(user_id)) -def _user_not_found_for_email(email): - return abort(404, 'User not found for email address: {}'.format(email)) +def _user_not_found_for_email(): + return abort(404, 'User not found for email address') def _create_reset_password_url(email): from utils.url_safe_token import generate_token - token = generate_token(email, current_app.config['SECRET_KEY'], current_app.config['DANGEROUS_SALT']) + import json + data = json.dumps({'email': email, 'created_at': str(datetime.now())}) + token = generate_token(data, current_app.config['SECRET_KEY'], current_app.config['DANGEROUS_SALT']) return current_app.config['ADMIN_BASE_URL'] + '/new-password/' + token diff --git a/config.py b/config.py index dd6fa5652..1226e09cc 100644 --- a/config.py +++ b/config.py @@ -48,7 +48,7 @@ class Config(object): Queue('email', Exchange('default'), routing_key='email'), Queue('sms-code', Exchange('default'), routing_key='sms-code'), Queue('email-code', Exchange('default'), routing_key='email-code'), - Queue('email-forgot-password', Exchange('default'), routing_key='email-forgot-password'), + Queue('email-reset-password', Exchange('default'), routing_key='email-reset-password'), Queue('process-job', Exchange('default'), routing_key='process-job'), Queue('bulk-sms', Exchange('default'), routing_key='bulk-sms'), Queue('bulk-email', Exchange('default'), routing_key='bulk-email'), diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 74af49d5a..8dc51b6b2 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -299,7 +299,7 @@ def test_get_user_by_email_not_found_returns_404(notify_api, assert resp.status_code == 404 json_resp = json.loads(resp.get_data(as_text=True)) assert json_resp['result'] == 'error' - assert json_resp['message'] == 'User not found for email address: {}'.format('no_user@digital.gov.uk') + assert json_resp['message'] == 'User not found for email address' def test_get_user_by_email_bad_url_returns_404(notify_api, @@ -430,63 +430,60 @@ def test_set_user_permissions_remove_old(notify_api, assert query.first().permission == MANAGE_SETTINGS -def test_send_reset_password_should_send_reset_password_link(notify_api, - sample_user, - mocker): +def test_send_user_reset_password_should_send_reset_password_link(notify_api, + sample_user, + mocker, + mock_encryption): with notify_api.test_request_context(): with notify_api.test_client() as client: mocker.patch('app.celery.tasks.email_reset_password.apply_async') data = json.dumps({'email': sample_user.email_address}) auth_header = create_authorization_header( - path=url_for('user.send_reset_password'), + path=url_for('user.send_user_reset_password'), method='POST', request_body=data) resp = client.post( - url_for('user.send_reset_password'), + url_for('user.send_user_reset_password'), data=data, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 204 - from app.user.rest import _create_reset_password_url - url = _create_reset_password_url(sample_user.email_address) - encrypted = encryption.encrypt({'to': sample_user.email_address, 'reset_password_url': url}) - app.celery.tasks.email_reset_password.apply_async.assert_called_once_with([encrypted], - queue='send-reset-password') + app.celery.tasks.email_reset_password.apply_async.assert_called_once_with(['something_encrypted'], + queue='email-reset-password') -def test_send_reset_password_should_return_400_when_user_doesnot_exist(notify_api, - mocker): +def test_send_user_reset_password_should_return_400_when_user_doesnot_exist(notify_api, + mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: bad_email_address = 'bad@email.gov.uk' data = json.dumps({'email': bad_email_address}) auth_header = create_authorization_header( - path=url_for('user.send_reset_password'), + path=url_for('user.send_user_reset_password'), method='POST', request_body=data) resp = client.post( - url_for('user.send_reset_password'), + url_for('user.send_user_reset_password'), data=data, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 404 - assert json.loads(resp.get_data(as_text=True))['message'] == 'User not found for email address: {}'.format( - bad_email_address) + assert json.loads(resp.get_data(as_text=True))['message'] == 'User not found for email address' -def test_send_reset_password_should_return_400_when_data_is_not_email_address(notify_api, mocker): +def test_send_user_reset_password_should_return_400_when_data_is_not_email_address(notify_api, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: bad_email_address = 'bad.email.gov.uk' data = json.dumps({'email': bad_email_address}) auth_header = create_authorization_header( - path=url_for('user.send_reset_password'), + path=url_for('user.send_user_reset_password'), method='POST', request_body=data) resp = client.post( - url_for('user.send_reset_password'), + url_for('user.send_user_reset_password'), data=data, headers=[('Content-Type', 'application/json'), auth_header]) From ba337374fd4c0bea811bf3016e6ee48ae7bb1f47 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 8 Mar 2016 14:33:06 +0000 Subject: [PATCH 4/8] - Remove password_changed_at from the update_dict in users_dao - Format dates in UserSchema - Properly formatted subject and message body for the password reset email - Add name to the message for reset password --- app/celery/tasks.py | 15 +++++++++++++-- app/dao/users_dao.py | 1 + app/schemas.py | 2 ++ app/user/rest.py | 1 + tests/app/celery/test_tasks.py | 6 ++++-- 5 files changed, 21 insertions(+), 4 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 968058fd0..5dbf89597 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -253,13 +253,24 @@ def email_invited_user(encrypted_invitation): current_app.logger.error(e) +def password_reset_message(name, url): + from string import Template + t = Template("Hi $user_name,\n\n" + "We received a request to reset your password on GOV.UK Notify.\n\n" + "If you didn't request this email, you can ignore it – your password has not been changed.\n\n" + "To reset your password, click this link:\n\n" + "$url") + return t.substitute(user_name=name, url=url) + + @notify_celery.task(name='email-reset-password') def email_reset_password(encrypted_reset_password_message): reset_password_message = encryption.decrypt(encrypted_reset_password_message) try: aws_ses_client.send_email(current_app.config['VERIFY_CODE_FROM_EMAIL_ADDRESS'], reset_password_message['to'], - "Reset password for GOV.UK Notify", - reset_password_message['reset_password_url']) + "Reset your GOV.UK Notify password", + password_reset_message(name=reset_password_message['name'], + url=reset_password_message['reset_password_url'])) except AwsSesClientException as e: current_app.logger.error(e) diff --git a/app/dao/users_dao.py b/app/dao/users_dao.py index 19e70302d..cfaf4cdb8 100644 --- a/app/dao/users_dao.py +++ b/app/dao/users_dao.py @@ -16,6 +16,7 @@ def save_model_user(usr, update_dict={}, pwd=None): if update_dict: if update_dict.get('id'): del update_dict['id'] + update_dict.pop('password_changed_at') db.session.query(User).filter_by(id=usr.id).update(update_dict) else: db.session.add(usr) diff --git a/app/schemas.py b/app/schemas.py index 2673f999a..b66898b2c 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -61,6 +61,8 @@ class BaseSchema(ma.ModelSchema): class UserSchema(BaseSchema): permissions = fields.Method("user_permissions", dump_only=True) + password_changed_at = field_for(models.User, 'password_changed_at', format='%Y-%m-%d %H:%M:%S.%f') + created_at = field_for(models.User, 'created_at', format='%Y-%m-%d %H:%M:%S.%f') def user_permissions(self, usr): retval = {} diff --git a/app/user/rest.py b/app/user/rest.py index fbedd452d..d0c4e9746 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -210,6 +210,7 @@ def send_user_reset_password(): return _user_not_found_for_email() reset_password_message = {'to': user_to_send_to.email_address, + 'name': user_to_send_to.name, 'reset_password_url': _create_reset_password_url(user_to_send_to.email_address)} email_reset_password.apply_async([encryption.encrypt(reset_password_message)], queue='email-reset-password') diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 0e2acef20..2b093fbba 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -514,6 +514,7 @@ def test_email_invited_user_should_send_email(notify_api, mocker): def test_email_reset_password_should_send_email(notify_api, mocker): with notify_api.test_request_context(): reset_password_message = {'to': 'someone@it.gov.uk', + 'name': 'Some One', 'reset_password_url': 'bah'} mocker.patch('app.aws_ses_client.send_email') @@ -521,8 +522,9 @@ def test_email_reset_password_should_send_email(notify_api, mocker): encrypted_message = encryption.encrypt(reset_password_message) email_reset_password(encrypted_message) - + message = tasks.password_reset_message(reset_password_message['name'], + reset_password_message['reset_password_url']) aws_ses_client.send_email(current_app.config['VERIFY_CODE_FROM_EMAIL_ADDRESS'], reset_password_message['to'], "Reset password for GOV.UK Notify", - reset_password_message['reset_password_url']) + message) From 6e17a015e8fca514a1001d7d7499a7e2c7ec40af Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 8 Mar 2016 15:20:34 +0000 Subject: [PATCH 5/8] Add missing import --- app/user/rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/user/rest.py b/app/user/rest.py index 458a1968b..8886429fd 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -1,5 +1,5 @@ from datetime import datetime -from flask import (jsonify, request, abort, Blueprint) +from flask import (jsonify, request, abort, Blueprint, current_app) from app import encryption from app.dao.users_dao import ( From 29a7289d1e5bb2db1cbeafbbd8be10cf17a6b642 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 8 Mar 2016 15:47:35 +0000 Subject: [PATCH 6/8] Use new email validation. Use logger.exception where it makes sense, not for SqlAlchemy errors as it give too much information away. --- app/celery/tasks.py | 8 ++++---- app/errors.py | 2 +- app/schemas.py | 9 +++++---- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index e5b6bc89d..3c4c3a491 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -197,7 +197,7 @@ def send_sms_code(encrypted_verification): try: firetext_client.send_sms(verification_message['to'], verification_message['secret_code']) except FiretextClientException as e: - current_app.logger.error(e) + current_app.logger.exception(e) @notify_celery.task(name='send-email-code') @@ -209,7 +209,7 @@ def send_email_code(encrypted_verification_message): "Verification code", verification_message['secret_code']) except AwsSesClientException as e: - current_app.logger.error(e) + current_app.logger.exception(e) # TODO: when placeholders in templates work, this will be a real template @@ -251,7 +251,7 @@ def email_invited_user(encrypted_invitation): subject_line, invitation_content) except AwsSesClientException as e: - current_app.logger.error(e) + current_app.logger.exception(e) def password_reset_message(name, url): @@ -274,4 +274,4 @@ def email_reset_password(encrypted_reset_password_message): password_reset_message(name=reset_password_message['name'], url=reset_password_message['reset_password_url'])) except AwsSesClientException as e: - current_app.logger.error(e) + current_app.logger.exception(e) diff --git a/app/errors.py b/app/errors.py index 890841cc3..465ed8acb 100644 --- a/app/errors.py +++ b/app/errors.py @@ -41,7 +41,7 @@ def register_errors(blueprint): @blueprint.app_errorhandler(500) def internal_server_error(e): if isinstance(e, str): - current_app.logger.error(e) + current_app.logger.exception(e) elif isinstance(e, Exception): current_app.logger.exception(e) return jsonify(result='error', message="Internal server error"), 500 diff --git a/app/schemas.py b/app/schemas.py index 4e8f023b9..36bd1995f 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -1,9 +1,8 @@ -import re from flask_marshmallow.fields import fields from . import ma from . import models from app.dao.permissions_dao import permission_dao -from marshmallow import (post_load, ValidationError, validates, validates_schema) +from marshmallow import (post_load, ValidationError, validates) from marshmallow_sqlalchemy import field_for from utils.recipients import ( validate_email_address, InvalidEmailError, @@ -202,8 +201,10 @@ class EmailDataSchema(ma.Schema): email = fields.Str(required=False) @validates('email') - def validate_to(self, value): - if not email_regex.match(value): + def validate_email(self, value): + try: + validate_email_address(value) + except InvalidEmailError: raise ValidationError('Invalid email') user_schema = UserSchema() From 114cfa6b171b5c0ec0341ffce893f8e1fc94f870 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 8 Mar 2016 17:46:00 +0000 Subject: [PATCH 7/8] Use the validation error message from the InvalidEmailError --- app/schemas.py | 12 ++++++------ tests/app/invite/test_invite_rest.py | 2 +- tests/app/notifications/test_rest.py | 2 +- tests/app/user/test_rest.py | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index 36bd1995f..6fdb67eff 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -138,8 +138,8 @@ class EmailNotificationSchema(NotificationSchema): def validate_to(self, value): try: validate_email_address(value) - except InvalidEmailError: - raise ValidationError('Invalid email') + except InvalidEmailError as e: + raise ValidationError(e.message) class SmsTemplateNotificationSchema(SmsNotificationSchema): @@ -176,8 +176,8 @@ class InvitedUserSchema(BaseSchema): def validate_to(self, value): try: validate_email_address(value) - except InvalidEmailError: - raise ValidationError('Invalid email') + except InvalidEmailError as e: + raise ValidationError(e.message) class PermissionSchema(BaseSchema): @@ -204,8 +204,8 @@ class EmailDataSchema(ma.Schema): def validate_email(self, value): try: validate_email_address(value) - except InvalidEmailError: - raise ValidationError('Invalid email') + except InvalidEmailError as e: + raise ValidationError(e.message) user_schema = UserSchema() user_schema_load_json = UserSchema(load_json=True) diff --git a/tests/app/invite/test_invite_rest.py b/tests/app/invite/test_invite_rest.py index c6299c740..2731efc82 100644 --- a/tests/app/invite/test_invite_rest.py +++ b/tests/app/invite/test_invite_rest.py @@ -90,7 +90,7 @@ def test_create_invited_user_invalid_email(notify_api, sample_service, mocker): assert response.status_code == 400 json_resp = json.loads(response.get_data(as_text=True)) assert json_resp['result'] == 'error' - assert json_resp['message'] == {'email_address': ['Invalid email']} + assert json_resp['message'] == {'email_address': ['Not a valid email address']} app.celery.tasks.email_invited_user.apply_async.assert_not_called() diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 5d8ed0b73..941e556a2 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -582,7 +582,7 @@ def test_should_reject_email_notification_with_bad_email(notify_api, sample_emai app.celery.tasks.send_email.apply_async.assert_not_called() assert response.status_code == 400 assert data['result'] == 'error' - assert data['message']['to'][0] == 'Invalid email' + assert data['message']['to'][0] == 'Not a valid email address' def test_should_reject_email_notification_with_template_id_that_cant_be_found( diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 8dc51b6b2..466cf0c50 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -488,4 +488,4 @@ def test_send_user_reset_password_should_return_400_when_data_is_not_email_addre headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 400 - assert json.loads(resp.get_data(as_text=True))['message'] == {'email': ['Invalid email']} + assert json.loads(resp.get_data(as_text=True))['message'] == {'email': ['Not a valid email address']} From d0c5977b9dfca83ffcfacdfdc683853ca7f3af60 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 9 Mar 2016 09:57:14 +0000 Subject: [PATCH 8/8] Fix extra space in test --- tests/app/notifications/test_rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index d7507fabd..510068494 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -582,7 +582,7 @@ def test_should_reject_email_notification_with_bad_email(notify_api, sample_emai app.celery.tasks.send_email.apply_async.assert_not_called() assert response.status_code == 400 assert data['result'] == 'error' - assert data['message']['to'][0] == 'Not a valid email address ' + assert data['message']['to'][0] == 'Not a valid email address' def test_should_reject_email_notification_with_template_id_that_cant_be_found(