From a186e277d7df6144c5f097d6e7318db0f068e7a1 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 4 Mar 2016 13:42:55 +0000 Subject: [PATCH 1/6] Sent count on jobs --- app/models.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models.py b/app/models.py index c2bbf4765..564760283 100644 --- a/app/models.py +++ b/app/models.py @@ -162,6 +162,7 @@ class Job(db.Model): onupdate=datetime.datetime.now) status = db.Column(db.Enum(*JOB_STATUS_TYPES, name='job_status_types'), nullable=False, default='pending') notification_count = db.Column(db.Integer, nullable=False) + notifications_sent = db.Column(db.Integer, nullable=False) processing_started = db.Column( db.DateTime, index=False, From c44aaf0fdc6edbda1de6357f888e859d72e39c77 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 4 Mar 2016 14:25:28 +0000 Subject: [PATCH 2/6] Capture the count of sent notifications for a job --- app/dao/notifications_dao.py | 14 ++++++---- app/models.py | 2 +- migrations/versions/0034_job_sent_count.py | 26 ++++++++++++++++++ tests/app/dao/test_notification_dao.py | 32 +++++++++++++++++++--- 4 files changed, 63 insertions(+), 11 deletions(-) create mode 100644 migrations/versions/0034_job_sent_count.py diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 16bf4ce7c..4f5f38c4a 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -1,10 +1,12 @@ from flask import current_app from app import db -from app.models import Notification +from app.models import Notification, Job from sqlalchemy import desc def dao_create_notification(notification): + if notification.job_id: + db.session.query(Job).update({Job.notifications_sent: Job.notifications_sent + 1}) db.session.add(notification) db.session.commit() @@ -19,12 +21,12 @@ def get_notification_for_job(service_id, job_id, notification_id): def get_notifications_for_job(service_id, job_id, page=1): - query = Notification.query.filter_by(service_id=service_id, job_id=job_id)\ - .order_by(desc(Notification.created_at))\ + query = Notification.query.filter_by(service_id=service_id, job_id=job_id) \ + .order_by(desc(Notification.created_at)) \ .paginate( - page=page, - per_page=current_app.config['PAGE_SIZE'] - ) + page=page, + per_page=current_app.config['PAGE_SIZE'] + ) return query diff --git a/app/models.py b/app/models.py index 564760283..56c484d0d 100644 --- a/app/models.py +++ b/app/models.py @@ -162,7 +162,7 @@ class Job(db.Model): onupdate=datetime.datetime.now) status = db.Column(db.Enum(*JOB_STATUS_TYPES, name='job_status_types'), nullable=False, default='pending') notification_count = db.Column(db.Integer, nullable=False) - notifications_sent = db.Column(db.Integer, nullable=False) + notifications_sent = db.Column(db.Integer, nullable=False, default=0) processing_started = db.Column( db.DateTime, index=False, diff --git a/migrations/versions/0034_job_sent_count.py b/migrations/versions/0034_job_sent_count.py new file mode 100644 index 000000000..b8d3f1a84 --- /dev/null +++ b/migrations/versions/0034_job_sent_count.py @@ -0,0 +1,26 @@ +"""empty message + +Revision ID: 0034_job_sent_count +Revises: 0033_correct_permission_enums +Create Date: 2016-03-04 13:44:32.217058 + +""" + +# revision identifiers, used by Alembic. +revision = '0034_job_sent_count' +down_revision = '0033_correct_permission_enums' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.add_column('jobs', sa.Column('notifications_sent', sa.Integer(), nullable=False)) + ### end Alembic commands ### + + +def downgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.drop_column('jobs', 'notifications_sent') + ### end Alembic commands ### diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 4ddffd47e..39a74e5b4 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -1,4 +1,4 @@ -from app.models import Notification +from app.models import Notification, Job from datetime import datetime from app.dao.notifications_dao import ( dao_create_notification, @@ -9,12 +9,37 @@ from app.dao.notifications_dao import ( ) -def test_save_notification(sample_template, sample_job): +def test_save_notification_and_increment_job(sample_template, sample_job): + + assert Notification.query.count() == 0 + data = { + 'to': '+44709123456', + 'job_id': sample_job.id, + 'service': sample_template.service, + 'template': sample_template, + 'created_at': datetime.utcnow() + } + + notification = Notification(**data) + dao_create_notification(notification) + + assert Notification.query.count() == 1 + notification_from_db = Notification.query.all()[0] + assert notification_from_db.id + assert data['to'] == notification_from_db.to + assert data['job_id'] == notification_from_db.job_id + assert data['service'] == notification_from_db.service + assert data['template'] == notification_from_db.template + assert data['created_at'] == notification_from_db.created_at + assert 'sent' == notification_from_db.status + assert Job.query.get(sample_job.id).notifications_sent == 1 + + +def test_save_notification_with_no_job(sample_template): assert Notification.query.count() == 0 data = { 'to': '+44709123456', - 'job': sample_job, 'service': sample_template.service, 'template': sample_template, 'created_at': datetime.utcnow() @@ -27,7 +52,6 @@ def test_save_notification(sample_template, sample_job): notification_from_db = Notification.query.all()[0] assert notification_from_db.id assert data['to'] == notification_from_db.to - assert data['job'] == notification_from_db.job assert data['service'] == notification_from_db.service assert data['template'] == notification_from_db.template assert data['created_at'] == notification_from_db.created_at From ae395b490e914f1ba0f562ed438b4a159cee3ee6 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 4 Mar 2016 15:54:43 +0000 Subject: [PATCH 3/6] Fixed bug where I forgot to update only the right job :-( --- app/dao/notifications_dao.py | 4 +++- tests/app/dao/test_notification_dao.py | 31 ++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 4f5f38c4a..f906f1aaf 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -6,7 +6,9 @@ from sqlalchemy import desc def dao_create_notification(notification): if notification.job_id: - db.session.query(Job).update({Job.notifications_sent: Job.notifications_sent + 1}) + db.session.query(Job).filter_by( + id=notification.job_id + ).update({Job.notifications_sent: Job.notifications_sent + 1}) db.session.add(notification) db.session.commit() diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 39a74e5b4..f3d032884 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -7,6 +7,7 @@ from app.dao.notifications_dao import ( get_notification_for_job, get_notifications_for_job ) +from tests.app.conftest import sample_job def test_save_notification_and_increment_job(sample_template, sample_job): @@ -35,6 +36,36 @@ def test_save_notification_and_increment_job(sample_template, sample_job): assert Job.query.get(sample_job.id).notifications_sent == 1 +def test_save_notification_and_increment_correct_job(notify_db, notify_db_session, sample_template): + + job_1 = sample_job(notify_db, notify_db_session, sample_template.service) + job_2 = sample_job(notify_db, notify_db_session, sample_template.service) + + assert Notification.query.count() == 0 + data = { + 'to': '+44709123456', + 'job_id': job_1.id, + 'service': sample_template.service, + 'template': sample_template, + 'created_at': datetime.utcnow() + } + + notification = Notification(**data) + dao_create_notification(notification) + + assert Notification.query.count() == 1 + notification_from_db = Notification.query.all()[0] + assert notification_from_db.id + assert data['to'] == notification_from_db.to + assert data['job_id'] == notification_from_db.job_id + assert data['service'] == notification_from_db.service + assert data['template'] == notification_from_db.template + assert data['created_at'] == notification_from_db.created_at + assert 'sent' == notification_from_db.status + assert Job.query.get(job_1.id).notifications_sent == 1 + assert Job.query.get(job_2.id).notifications_sent == 0 + + def test_save_notification_with_no_job(sample_template): assert Notification.query.count() == 0 From b3f4e4042104a2df36f0c8e1d854be8084b0e7a9 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 4 Mar 2016 07:03:15 +0000 Subject: [PATCH 4/6] Strip HTML from template content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Templates are created in the admin app and persisted in the API. They are consumed: - in the admin app, by requesting them from the API - in the API, by loading them from the database There are two potential places where unescaped HTML could be sent to a user: - when the admin app is previewing a template (it has to render the template as markup in order to show the placeholders) - in the body of an email For all consumers to have confidence that the templates are safe, it makes sense to santitise them at the point of creation (and modification). This also avoids any performance issues that could come from doing it at the point of requesting a template. In the future they could be created by a direct API call, bypassing the admin app. Therefore it makes sense for the API to sanitise them. The commit sanitises templates using a Mozilla’s Bleach library[1]. It is configured to get the text content of the template, minus any HTML tags. It is not using a regex because[2]. 1. https://github.com/mozilla/bleach 2. http://stackoverflow.com/questions/1732348/regex-match-open-tags-except-xhtml-self-contained-tags/1732454#1732454 --- app/template/rest.py | 7 +++++++ requirements.txt | 1 + tests/app/template/test_rest.py | 8 ++++---- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/app/template/rest.py b/app/template/rest.py index 26ae98502..8da697667 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -4,6 +4,7 @@ from flask import ( request, current_app ) +import bleach from sqlalchemy.exc import IntegrityError from app.dao.templates_dao import ( @@ -34,6 +35,7 @@ def create_template(service_id): if errors: return jsonify(result="error", message=errors), 400 new_template.service = fetched_service + new_template.content = _strip_html(new_template.content) try: dao_create_template(new_template) except IntegrityError as ex: @@ -55,6 +57,7 @@ def update_template(service_id, template_id): current_data = dict(template_schema.dump(fetched_template).data.items()) current_data.update(request.get_json()) + current_data['content'] = _strip_html(current_data['content']) update_dict, errors = template_schema.load(current_data) if errors: @@ -79,3 +82,7 @@ def get_template_by_id_and_service_id(service_id, template_id): return jsonify(data=data) else: return jsonify(result="error", message="Template not found"), 404 + + +def _strip_html(content): + return bleach.clean(content, tags=[], strip=True) diff --git a/requirements.txt b/requirements.txt index c41e2e6f4..254a2f3a5 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,4 @@ +bleach==1.4.2 Flask==0.10.1 Flask-Script==2.0.5 Flask-Migrate==1.3.1 diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 0ba8c0a68..670862bb5 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -10,7 +10,7 @@ def test_should_create_a_new_sms_template_for_a_service(notify_api, sample_servi data = { 'name': 'my template', 'template_type': 'sms', - 'content': 'template content', + 'content': 'template content', 'service': str(sample_service.id) } data = json.dumps(data) @@ -42,7 +42,7 @@ def test_should_create_a_new_email_template_for_a_service(notify_api, sample_ser 'name': 'my template', 'template_type': 'email', 'subject': 'subject', - 'content': 'template content', + 'content': 'template content', 'service': str(sample_service.id) } data = json.dumps(data) @@ -222,7 +222,7 @@ def test_should_be_able_to_update_a_template(notify_api, sample_service): json_resp = json.loads(create_response.get_data(as_text=True)) assert json_resp['data']['name'] == 'my template' data = { - 'name': 'my template has a new name' + 'content': 'my template has new content ' } data = json.dumps(data) auth_header = create_authorization_header( @@ -239,7 +239,7 @@ def test_should_be_able_to_update_a_template(notify_api, sample_service): assert update_response.status_code == 200 update_json_resp = json.loads(update_response.get_data(as_text=True)) - assert update_json_resp['data']['name'] == 'my template has a new name' + assert update_json_resp['data']['content'] == 'my template has new content alert("foo")' def test_should_be_able_to_get_all_templates_for_a_service(notify_api, sample_service): From 4f8c2d31a5b046a5e430b33b0115a303aa00c904 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 7 Mar 2016 15:01:40 +0000 Subject: [PATCH 5/6] Capture logged in at when password is verified --- app/user/rest.py | 4 +++- tests/app/user/test_rest_verify.py | 5 ++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/user/rest.py b/app/user/rest.py index 9977410db..6287522b6 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, current_app) +from flask import (jsonify, request, abort, Blueprint) from app import encryption from app.dao.users_dao import ( @@ -77,6 +77,8 @@ def verify_user_password(user_id): result="error", message={'password': ['Required field missing data']}), 400 if user_to_verify.check_password(txt_pwd): + user_to_verify.logged_in_at = datetime.utcnow() + save_model_user(user_to_verify) reset_failed_login_count(user_to_verify) return jsonify({}), 204 else: diff --git a/tests/app/user/test_rest_verify.py b/tests/app/user/test_rest_verify.py index 10310627c..98d9b65fd 100644 --- a/tests/app/user/test_rest_verify.py +++ b/tests/app/user/test_rest_verify.py @@ -2,10 +2,11 @@ import json import moto from datetime import (datetime, timedelta) from flask import url_for -from app.models import (VerifyCode) +from app.models import (VerifyCode, User) import app.celery.tasks from app import db, encryption from tests import create_authorization_header +from freezegun import freeze_time def test_user_verify_code_sms(notify_api, @@ -137,6 +138,7 @@ def test_user_verify_code_email_expired_code(notify_api, assert not VerifyCode.query.first().code_used +@freeze_time("2016-01-01 10:00:00.000000") def test_user_verify_password(notify_api, notify_db, notify_db_session, @@ -156,6 +158,7 @@ def test_user_verify_password(notify_api, data=data, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 204 + User.query.get(sample_user.id).logged_in_at == datetime.utcnow() def test_user_verify_password_invalid_password(notify_api, From 8d8abb524d8cd48b39a14d2af72813a4fdf9a5a7 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 8 Mar 2016 09:12:33 +0000 Subject: [PATCH 6/6] Add script to set notifications sent count on jobs table. --- migrations/versions/0034_job_sent_count.py | 2 +- .../versions/0035_default_sent_count.py | 27 +++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 migrations/versions/0035_default_sent_count.py diff --git a/migrations/versions/0034_job_sent_count.py b/migrations/versions/0034_job_sent_count.py index b8d3f1a84..d741d1b88 100644 --- a/migrations/versions/0034_job_sent_count.py +++ b/migrations/versions/0034_job_sent_count.py @@ -16,7 +16,7 @@ import sqlalchemy as sa def upgrade(): ### commands auto generated by Alembic - please adjust! ### - op.add_column('jobs', sa.Column('notifications_sent', sa.Integer(), nullable=False)) + op.add_column('jobs', sa.Column('notifications_sent', sa.Integer(), nullable=True)) ### end Alembic commands ### diff --git a/migrations/versions/0035_default_sent_count.py b/migrations/versions/0035_default_sent_count.py new file mode 100644 index 000000000..aac9524a6 --- /dev/null +++ b/migrations/versions/0035_default_sent_count.py @@ -0,0 +1,27 @@ +"""empty message + +Revision ID: 0035_default_sent_count +Revises: 0034_job_sent_count +Create Date: 2016-03-08 09:08:55.721654 + +""" + +# revision identifiers, used by Alembic. +revision = '0035_default_sent_count' +down_revision = '0034_job_sent_count' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + op.execute('update jobs set notifications_sent = notification_count') + op.alter_column('jobs', 'notifications_sent', + existing_type=sa.INTEGER(), + nullable=False) + + +def downgrade(): + op.alter_column('jobs', 'notifications_sent', + existing_type=sa.INTEGER(), + nullable=True)