From 2b645f490a675f0c53fb889b807fdd2a7cd7beb3 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 28 Jun 2016 15:17:36 +0100 Subject: [PATCH 1/5] move get_all_notifications_for_service and get_all_notifications_for_job moved from notifications/rest -> service/rest and job/rest respectively endpoint routes not affected removed requires_admin decorator - that should be set by nginx config as opposed to python code --- app/authentication/auth.py | 11 -- app/job/rest.py | 35 +++++- app/notifications/rest.py | 71 +----------- app/service/rest.py | 46 ++++++-- app/utils.py | 13 +++ tests/app/job/test_rest.py | 113 +++++++++++++++++- tests/app/notifications/test_rest.py | 167 --------------------------- tests/app/service/test_rest.py | 34 +++++- 8 files changed, 225 insertions(+), 265 deletions(-) create mode 100644 app/utils.py diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 1c3e7891d..bb1f2e9fb 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -59,14 +59,3 @@ def fetch_client(client): "client": client, "secret": get_unsigned_secrets(client) } - - -def require_admin(): - def wrap(func): - @wraps(func) - def wrap_func(*args, **kwargs): - if not api_user['client'] == current_app.config.get('ADMIN_CLIENT_USER_NAME'): - abort(403) - return func(*args, **kwargs) - return wrap_func - return wrap diff --git a/app/job/rest.py b/app/job/rest.py index fc3d49943..8f6512404 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -1,7 +1,8 @@ from flask import ( Blueprint, jsonify, - request + request, + current_app ) from app.dao.jobs_dao import ( @@ -15,11 +16,14 @@ from app.dao.services_dao import ( ) from app.dao.templates_dao import (dao_get_template_by_id) +from app.dao.notifications_dao import get_notifications_for_job -from app.schemas import job_schema, unarchived_template_schema +from app.schemas import job_schema, unarchived_template_schema, notifications_filter_schema, notification_status_schema from app.celery.tasks import process_job +from app.utils import pagination_links + job = Blueprint('job', __name__, url_prefix='/service//job') from app.errors import ( @@ -37,6 +41,33 @@ def get_job_by_service_and_job_id(service_id, job_id): return jsonify(data=data) +@job.route('//notifications', methods=['GET']) +def get_all_notifications_for_service_job(service_id, job_id): + data = notifications_filter_schema.load(request.args).data + page = data['page'] if 'page' in data else 1 + page_size = data['page_size'] if 'page_size' in data else current_app.config.get('PAGE_SIZE') + + pagination = get_notifications_for_job( + service_id, + job_id, + filter_dict=data, + page=page, + page_size=page_size) + kwargs = request.args.to_dict() + kwargs['service_id'] = service_id + kwargs['job_id'] = job_id + return jsonify( + notifications=notification_status_schema.dump(pagination.items, many=True).data, + page_size=page_size, + total=pagination.total, + links=pagination_links( + pagination, + '.get_all_notifications_for_service_job', + **kwargs + ) + ), 200 + + @job.route('', methods=['GET']) def get_jobs_by_service(service_id): if request.args.get('limit_days'): diff --git a/app/notifications/rest.py b/app/notifications/rest.py index fc82668bc..22186c341 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -5,14 +5,12 @@ from flask import ( jsonify, request, current_app, - url_for, json ) from notifications_utils.recipients import allowed_to_send_to, first_column_heading from notifications_utils.template import Template from app.clients.email.aws_ses import get_aws_responses from app import api_user, encryption, create_uuid, DATETIME_FORMAT, DATE_FORMAT, statsd_client -from app.authentication.auth import require_admin from app.dao import ( templates_dao, services_dao, @@ -32,6 +30,7 @@ from app.schemas import ( unarchived_template_schema ) from app.celery.tasks import send_sms, send_email +from app.utils import pagination_links notifications = Blueprint('notifications', __name__) @@ -199,74 +198,6 @@ def get_all_notifications(): ), 200 -@notifications.route('/service//notifications', methods=['GET']) -@require_admin() -def get_all_notifications_for_service(service_id): - data = notifications_filter_schema.load(request.args).data - page = data['page'] if 'page' in data else 1 - page_size = data['page_size'] if 'page_size' in data else current_app.config.get('PAGE_SIZE') - limit_days = data.get('limit_days') - - pagination = notifications_dao.get_notifications_for_service( - service_id, - filter_dict=data, - page=page, - page_size=page_size, - limit_days=limit_days) - kwargs = request.args.to_dict() - kwargs['service_id'] = service_id - return jsonify( - notifications=notification_status_schema.dump(pagination.items, many=True).data, - page_size=page_size, - total=pagination.total, - links=pagination_links( - pagination, - '.get_all_notifications_for_service', - **kwargs - ) - ), 200 - - -@notifications.route('/service//job//notifications', methods=['GET']) -@require_admin() -def get_all_notifications_for_service_job(service_id, job_id): - data = notifications_filter_schema.load(request.args).data - page = data['page'] if 'page' in data else 1 - page_size = data['page_size'] if 'page_size' in data else current_app.config.get('PAGE_SIZE') - - pagination = notifications_dao.get_notifications_for_job( - service_id, - job_id, - filter_dict=data, - page=page, - page_size=page_size) - kwargs = request.args.to_dict() - kwargs['service_id'] = service_id - kwargs['job_id'] = job_id - return jsonify( - notifications=notification_status_schema.dump(pagination.items, many=True).data, - page_size=page_size, - total=pagination.total, - links=pagination_links( - pagination, - '.get_all_notifications_for_service_job', - **kwargs - ) - ), 200 - - -def pagination_links(pagination, endpoint, **kwargs): - if 'page' in kwargs: - kwargs.pop('page', None) - links = dict() - if pagination.has_prev: - links['prev'] = url_for(endpoint, page=pagination.prev_num, **kwargs) - if pagination.has_next: - links['next'] = url_for(endpoint, page=pagination.next_num, **kwargs) - links['last'] = url_for(endpoint, page=pagination.pages, **kwargs) - return links - - @notifications.route('/notifications/', methods=['POST']) def send_notification(notification_type): if notification_type not in ['sms', 'email']: diff --git a/app/service/rest.py b/app/service/rest.py index e94107260..94d3aabd8 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -1,14 +1,11 @@ -from datetime import ( - datetime, - date -) +from datetime import date from flask import ( jsonify, request, - Blueprint + Blueprint, + current_app ) - from sqlalchemy.orm.exc import NoResultFound from app.dao.api_key_dao import ( @@ -26,19 +23,19 @@ from app.dao.services_dao import ( dao_add_user_to_service, dao_remove_user_from_service ) - +from app.dao import notifications_dao from app.dao.provider_statistics_dao import get_fragment_count - from app.dao.users_dao import get_model_users - from app.schemas import ( service_schema, api_key_schema, user_schema, from_to_date_schema, - permission_schema + permission_schema, + notification_status_schema, + notifications_filter_schema, ) - +from app.utils import pagination_links from app.errors import ( register_errors, InvalidRequest @@ -208,3 +205,30 @@ def get_service_history(service_id): 'events': events_data} return jsonify(data=data) + + +@service.route('//notifications', methods=['GET']) +def get_all_notifications_for_service(service_id): + data = notifications_filter_schema.load(request.args).data + page = data['page'] if 'page' in data else 1 + page_size = data['page_size'] if 'page_size' in data else current_app.config.get('PAGE_SIZE') + limit_days = data.get('limit_days') + + pagination = notifications_dao.get_notifications_for_service( + service_id, + filter_dict=data, + page=page, + page_size=page_size, + limit_days=limit_days) + kwargs = request.args.to_dict() + kwargs['service_id'] = service_id + return jsonify( + notifications=notification_status_schema.dump(pagination.items, many=True).data, + page_size=page_size, + total=pagination.total, + links=pagination_links( + pagination, + '.get_all_notifications_for_service', + **kwargs + ) + ), 200 diff --git a/app/utils.py b/app/utils.py new file mode 100644 index 000000000..fb65fef48 --- /dev/null +++ b/app/utils.py @@ -0,0 +1,13 @@ +from flask import url_for + + +def pagination_links(pagination, endpoint, **kwargs): + if 'page' in kwargs: + kwargs.pop('page', None) + links = dict() + if pagination.has_prev: + links['prev'] = url_for(endpoint, page=pagination.prev_num, **kwargs) + if pagination.has_next: + links['next'] = url_for(endpoint, page=pagination.next_num, **kwargs) + links['last'] = url_for(endpoint, page=pagination.pages, **kwargs) + return links diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index 761cecbf7..ea65bb447 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -2,11 +2,16 @@ import json import uuid from datetime import datetime, timedelta +import pytest + import app.celery.tasks from tests import create_authorization_header -from tests.app.conftest import sample_job as create_job +from tests.app.conftest import ( + sample_job as create_job, + sample_notification as create_sample_notification) from app.dao.templates_dao import dao_update_template +from app.models import NOTIFICATION_STATUS_TYPES def test_get_jobs(notify_api, notify_db, notify_db_session, sample_template): @@ -239,3 +244,109 @@ def _setup_jobs(notify_db, notify_db_session, template, number_of_jobs=5): notify_db_session, service=template.service, template=template) + + +def test_get_all_notifications_for_job_in_order_of_job_number(notify_api, + notify_db, + notify_db_session, + sample_service): + with notify_api.test_request_context(), notify_api.test_client() as client: + main_job = create_job(notify_db, notify_db_session, service=sample_service) + another_job = create_job(notify_db, notify_db_session, service=sample_service) + + notification_1 = create_sample_notification( + notify_db, + notify_db_session, + job=main_job, + to_field="1", + created_at=datetime.utcnow(), + job_row_number=1 + ) + notification_2 = create_sample_notification( + notify_db, + notify_db_session, + job=main_job, + to_field="2", + created_at=datetime.utcnow(), + job_row_number=2 + ) + notification_3 = create_sample_notification( + notify_db, + notify_db_session, + job=main_job, + to_field="3", + created_at=datetime.utcnow(), + job_row_number=3 + ) + create_sample_notification(notify_db, notify_db_session, job=another_job) + + auth_header = create_authorization_header() + + response = client.get( + path='/service/{}/job/{}/notifications'.format(sample_service.id, main_job.id), + headers=[auth_header]) + + resp = json.loads(response.get_data(as_text=True)) + assert len(resp['notifications']) == 3 + assert resp['notifications'][0]['to'] == notification_1.to + assert resp['notifications'][0]['job_row_number'] == notification_1.job_row_number + assert resp['notifications'][1]['to'] == notification_2.to + assert resp['notifications'][1]['job_row_number'] == notification_2.job_row_number + assert resp['notifications'][2]['to'] == notification_3.to + assert resp['notifications'][2]['job_row_number'] == notification_3.job_row_number + assert response.status_code == 200 + + +@pytest.mark.parametrize( + "expected_notification_count, status_args", + [ + ( + 1, + '?status={}'.format(NOTIFICATION_STATUS_TYPES[0]) + ), + ( + 0, + '?status={}'.format(NOTIFICATION_STATUS_TYPES[1]) + ), + ( + 1, + '?status={}&status={}&status={}'.format( + *NOTIFICATION_STATUS_TYPES[0:3] + ) + ), + ( + 0, + '?status={}&status={}&status={}'.format( + *NOTIFICATION_STATUS_TYPES[3:6] + ) + ), + ] +) +def test_get_all_notifications_for_job_filtered_by_status( + notify_api, + notify_db, + notify_db_session, + sample_service, + expected_notification_count, + status_args +): + with notify_api.test_request_context(), notify_api.test_client() as client: + job = create_job(notify_db, notify_db_session, service=sample_service) + + create_sample_notification( + notify_db, + notify_db_session, + job=job, + to_field="1", + created_at=datetime.utcnow(), + status=NOTIFICATION_STATUS_TYPES[0], + job_row_number=1 + ) + + response = client.get( + path='/service/{}/job/{}/notifications{}'.format(sample_service.id, job.id, status_args), + headers=[create_authorization_header()] + ) + resp = json.loads(response.get_data(as_text=True)) + assert len(resp['notifications']) == expected_notification_count + assert response.status_code == 200 diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 5759254b1..98e1b5f03 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -1,15 +1,12 @@ from datetime import datetime, timedelta import uuid -import pytest from flask import json import app.celery.tasks -from app.models import NOTIFICATION_STATUS_TYPES from app.dao.notifications_dao import get_notification_by_id, dao_get_notification_statistics_for_service from tests import create_authorization_header from tests.app.conftest import sample_notification as create_sample_notification -from tests.app.conftest import sample_job as create_sample_job from tests.app.conftest import sample_service as create_sample_service @@ -137,170 +134,6 @@ def test_get_all_notifications_newest_first(notify_api, notify_db, notify_db_ses assert response.status_code == 200 -def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notify_db_session): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - service_1 = create_sample_service(notify_db, notify_db_session, service_name="1", email_from='1') - service_2 = create_sample_service(notify_db, notify_db_session, service_name="2", email_from='2') - - create_sample_notification(notify_db, notify_db_session, service=service_2) - - notification_1 = create_sample_notification(notify_db, notify_db_session, service=service_1) - notification_2 = create_sample_notification(notify_db, notify_db_session, service=service_1) - notification_3 = create_sample_notification(notify_db, notify_db_session, service=service_1) - - auth_header = create_authorization_header() - - response = client.get( - path='/service/{}/notifications'.format(service_1.id), - headers=[auth_header]) - - resp = json.loads(response.get_data(as_text=True)) - assert len(resp['notifications']) == 3 - assert resp['notifications'][0]['to'] == notification_3.to - assert resp['notifications'][1]['to'] == notification_2.to - assert resp['notifications'][2]['to'] == notification_1.to - assert response.status_code == 200 - - -def test_get_all_notifications_for_job_in_order_of_job_number(notify_api, - notify_db, - notify_db_session, - sample_service): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - main_job = create_sample_job(notify_db, notify_db_session, service=sample_service) - another_job = create_sample_job(notify_db, notify_db_session, service=sample_service) - - notification_1 = create_sample_notification( - notify_db, - notify_db_session, - job=main_job, - to_field="1", - created_at=datetime.utcnow(), - job_row_number=1 - ) - notification_2 = create_sample_notification( - notify_db, - notify_db_session, - job=main_job, - to_field="2", - created_at=datetime.utcnow(), - job_row_number=2 - ) - notification_3 = create_sample_notification( - notify_db, - notify_db_session, - job=main_job, - to_field="3", - created_at=datetime.utcnow(), - job_row_number=3 - ) - create_sample_notification(notify_db, notify_db_session, job=another_job) - - auth_header = create_authorization_header() - - response = client.get( - path='/service/{}/job/{}/notifications'.format(sample_service.id, main_job.id), - headers=[auth_header]) - - resp = json.loads(response.get_data(as_text=True)) - assert len(resp['notifications']) == 3 - assert resp['notifications'][0]['to'] == notification_1.to - assert resp['notifications'][0]['job_row_number'] == notification_1.job_row_number - assert resp['notifications'][1]['to'] == notification_2.to - assert resp['notifications'][1]['job_row_number'] == notification_2.job_row_number - assert resp['notifications'][2]['to'] == notification_3.to - assert resp['notifications'][2]['job_row_number'] == notification_3.job_row_number - assert response.status_code == 200 - - -@pytest.mark.parametrize( - "expected_notification_count, status_args", - [ - ( - 1, - '?status={}'.format(NOTIFICATION_STATUS_TYPES[0]) - ), - ( - 0, - '?status={}'.format(NOTIFICATION_STATUS_TYPES[1]) - ), - ( - 1, - '?status={}&status={}&status={}'.format( - *NOTIFICATION_STATUS_TYPES[0:3] - ) - ), - ( - 0, - '?status={}&status={}&status={}'.format( - *NOTIFICATION_STATUS_TYPES[3:6] - ) - ), - ] -) -def test_get_all_notifications_for_job_filtered_by_status( - notify_api, - notify_db, - notify_db_session, - sample_service, - expected_notification_count, - status_args -): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - job = create_sample_job(notify_db, notify_db_session, service=sample_service) - - create_sample_notification( - notify_db, - notify_db_session, - job=job, - to_field="1", - created_at=datetime.utcnow(), - status=NOTIFICATION_STATUS_TYPES[0], - job_row_number=1 - ) - - response = client.get( - path='/service/{}/job/{}/notifications{}'.format(sample_service.id, job.id, status_args), - headers=[create_authorization_header()] - ) - resp = json.loads(response.get_data(as_text=True)) - assert len(resp['notifications']) == expected_notification_count - assert response.status_code == 200 - - -def test_should_not_get_notifications_by_service_with_client_credentials(notify_api, sample_api_key): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header(service_id=sample_api_key.service.id) - - response = client.get( - '/service/{}/notifications'.format(sample_api_key.service.id), - headers=[auth_header]) - - resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 403 - assert resp['result'] == 'error' - assert resp['message'] == 'Forbidden, invalid authentication token provided' - - -def test_should_not_get_notifications_by_job_and_service_with_client_credentials(notify_api, sample_job): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header(service_id=sample_job.service.id) - - response = client.get( - '/service/{}/job/{}/notifications'.format(sample_job.service.id, sample_job.id), - headers=[auth_header]) - - resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 403 - assert resp['result'] == 'error' - assert resp['message'] == 'Forbidden, invalid authentication token provided' - - def test_should_reject_invalid_page_param(notify_api, sample_email_template): with notify_api.test_request_context(): with notify_api.test_client() as client: diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 7dc43853a..f332ce495 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -6,9 +6,12 @@ from app.dao.users_dao import save_model_user from app.dao.services_dao import dao_remove_user_from_service from app.models import User from tests import create_authorization_header -from tests.app.conftest import sample_service as create_sample_service -from tests.app.conftest import sample_service_permission as create_sample_service_permission -from tests.app.conftest import sample_user as create_sample_user +from tests.app.conftest import ( + sample_service as create_sample_service, + sample_service_permission as create_sample_service_permission, + sample_user as create_sample_user, + sample_notification as create_sample_notification +) def test_get_service_list(notify_api, service_factory): @@ -1001,3 +1004,28 @@ def test_set_reply_to_email_for_service(notify_api, sample_service): result = json.loads(resp.get_data(as_text=True)) assert resp.status_code == 200 assert result['data']['reply_to_email_address'] == 'reply_test@service.gov.uk' + + +def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notify_db_session): + with notify_api.test_request_context(), notify_api.test_client() as client: + service_1 = create_sample_service(notify_db, notify_db_session, service_name="1", email_from='1') + service_2 = create_sample_service(notify_db, notify_db_session, service_name="2", email_from='2') + + create_sample_notification(notify_db, notify_db_session, service=service_2) + + notification_1 = create_sample_notification(notify_db, notify_db_session, service=service_1) + notification_2 = create_sample_notification(notify_db, notify_db_session, service=service_1) + notification_3 = create_sample_notification(notify_db, notify_db_session, service=service_1) + + auth_header = create_authorization_header() + + response = client.get( + path='/service/{}/notifications'.format(service_1.id), + headers=[auth_header]) + + resp = json.loads(response.get_data(as_text=True)) + assert len(resp['notifications']) == 3 + assert resp['notifications'][0]['to'] == notification_3.to + assert resp['notifications'][1]['to'] == notification_2.to + assert resp['notifications'][2]['to'] == notification_1.to + assert response.status_code == 200 From 18b30de452522d9ec0af6ec4d3ae0893907efe78 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 28 Jun 2016 17:01:00 +0100 Subject: [PATCH 2/5] fix calling init_app twice this was causing flask decorators like auth check to be mounted twice --- app/__init__.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/__init__.py b/app/__init__.py index daf382713..9a57ed474 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -47,7 +47,6 @@ def create_app(app_name=None): init_app(application) db.init_app(application) ma.init_app(application) - init_app(application) logging.init_app(application) statsd_client.init_app(application) firetext_client.init_app(application, statsd_client=statsd_client) From adbe02783d0ee1b32e6d12eafa911055fdb0c756 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 29 Jun 2016 14:15:32 +0100 Subject: [PATCH 3/5] refactor authentication code moved api_key secret manipulation (generating and getting) into authentiation/utils, and added a property on the model, to facilitate easier matching of authenticated requests and the api keys they used --- app/authentication/auth.py | 4 +--- app/authentication/utils.py | 12 +++++++++++ app/dao/api_key_dao.py | 21 ++++--------------- app/models.py | 7 +++++-- .../app/authentication/test_authentication.py | 12 ++--------- tests/app/authentication/test_utils.py | 8 +++++++ tests/app/dao/test_api_key_dao.py | 16 ++++---------- 7 files changed, 36 insertions(+), 44 deletions(-) create mode 100644 app/authentication/utils.py create mode 100644 tests/app/authentication/test_utils.py diff --git a/app/authentication/auth.py b/app/authentication/auth.py index bb1f2e9fb..193afa1cc 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -1,10 +1,8 @@ from flask import request, jsonify, _request_ctx_stack, current_app from notifications_python_client.authentication import decode_jwt_token, get_token_issuer from notifications_python_client.errors import TokenDecodeError, TokenExpiredError -from werkzeug.exceptions import abort + from app.dao.api_key_dao import get_unsigned_secrets -from app import api_user -from functools import wraps def authentication_response(message, code): diff --git a/app/authentication/utils.py b/app/authentication/utils.py new file mode 100644 index 000000000..fa10f44b5 --- /dev/null +++ b/app/authentication/utils.py @@ -0,0 +1,12 @@ +from flask import current_app +from itsdangerous import URLSafeSerializer + + +def get_secret(secret): + serializer = URLSafeSerializer(current_app.config.get('SECRET_KEY')) + return serializer.loads(secret, salt=current_app.config.get('DANGEROUS_SALT')) + + +def generate_secret(token): + serializer = URLSafeSerializer(current_app.config.get('SECRET_KEY')) + return serializer.dumps(str(token), current_app.config.get('DANGEROUS_SALT')) diff --git a/app/dao/api_key_dao.py b/app/dao/api_key_dao.py index 2b7e040ff..04299c9a7 100644 --- a/app/dao/api_key_dao.py +++ b/app/dao/api_key_dao.py @@ -1,9 +1,6 @@ import uuid from datetime import datetime -from flask import current_app -from itsdangerous import URLSafeSerializer - from app import db from app.models import ApiKey @@ -11,6 +8,7 @@ from app.dao.dao_utils import ( transactional, version_class ) +from app.authentication.utils import generate_secret @transactional @@ -18,7 +16,7 @@ from app.dao.dao_utils import ( def save_model_api_key(api_key): if not api_key.id: api_key.id = uuid.uuid4() # must be set now so version history model can use same id - api_key.secret = _generate_secret() + api_key.secret = generate_secret(uuid.uuid4()) db.session.add(api_key) @@ -41,7 +39,7 @@ def get_unsigned_secrets(service_id): This method can only be exposed to the Authentication of the api calls. """ api_keys = ApiKey.query.filter_by(service_id=service_id, expiry_date=None).all() - keys = [_get_secret(x.secret) for x in api_keys] + keys = [x.unsigned_secret for x in api_keys] return keys @@ -50,15 +48,4 @@ def get_unsigned_secret(key_id): This method can only be exposed to the Authentication of the api calls. """ api_key = ApiKey.query.filter_by(id=key_id, expiry_date=None).one() - return _get_secret(api_key.secret) - - -def _generate_secret(): - token = uuid.uuid4() - serializer = URLSafeSerializer(current_app.config.get('SECRET_KEY')) - return serializer.dumps(str(token), current_app.config.get('DANGEROUS_SALT')) - - -def _get_secret(signed_secret): - serializer = URLSafeSerializer(current_app.config.get('SECRET_KEY')) - return serializer.loads(signed_secret, salt=current_app.config.get('DANGEROUS_SALT')) + return api_key.unsigned_secret diff --git a/app/models.py b/app/models.py index 06681ec57..0d2952e2b 100644 --- a/app/models.py +++ b/app/models.py @@ -5,14 +5,13 @@ from sqlalchemy.dialects.postgresql import ( UUID, JSON ) - from sqlalchemy import UniqueConstraint from app.encryption import ( hashpw, check_hash ) - +from app.authentication.utils import get_secret from app import ( db, encryption @@ -135,6 +134,10 @@ class ApiKey(db.Model, Versioned): UniqueConstraint('service_id', 'name', name='uix_service_to_key_name'), ) + @property + def unsigned_secret(self): + return get_secret(self.secret) + KEY_TYPE_NORMAL = 'normal' KEY_TYPE_TEAM = 'team' diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index 12d7e4681..03c1d0899 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -1,9 +1,8 @@ -import uuid -from datetime import datetime, timedelta +from datetime import datetime from notifications_python_client.authentication import create_jwt_token from flask import json, current_app from app.dao.api_key_dao import get_unsigned_secrets, save_model_api_key, get_unsigned_secret, expire_api_key -from app.models import ApiKey, KEY_TYPE_NORMAL +from app.models import ApiKey, KEY_TYPE_NORMAL, KEY_TYPE_TEAM def test_should_not_allow_request_with_no_token(notify_api): @@ -90,13 +89,6 @@ def test_should_allow_valid_token_when_service_has_multiple_keys(notify_api, sam assert response.status_code == 200 -JSON_BODY = json.dumps({ - "key1": "value1", - "key2": "value2", - "key3": "value3" -}) - - def test_authentication_passes_admin_client_token(notify_api, notify_db, notify_db_session, diff --git a/tests/app/authentication/test_utils.py b/tests/app/authentication/test_utils.py new file mode 100644 index 000000000..c99ed734c --- /dev/null +++ b/tests/app/authentication/test_utils.py @@ -0,0 +1,8 @@ +from app.authentication.utils import generate_secret, get_secret + + +def test_secret_is_signed_and_can_be_read_again(notify_api): + with notify_api.test_request_context(): + signed_secret = generate_secret('some_uuid') + assert signed_secret != 'some_uuid' + assert 'some_uuid' == get_secret(signed_secret) diff --git a/tests/app/dao/test_api_key_dao.py b/tests/app/dao/test_api_key_dao.py index 233966b4f..555770125 100644 --- a/tests/app/dao/test_api_key_dao.py +++ b/tests/app/dao/test_api_key_dao.py @@ -4,23 +4,15 @@ import pytest from sqlalchemy.exc import IntegrityError from sqlalchemy.orm.exc import NoResultFound +from app.authentication.utils import get_secret from app.dao.api_key_dao import (save_model_api_key, get_model_api_keys, get_unsigned_secrets, get_unsigned_secret, - _generate_secret, - _get_secret, expire_api_key) + expire_api_key) from app.models import ApiKey, KEY_TYPE_NORMAL -def test_secret_is_signed_and_can_be_read_again(notify_api, mocker): - with notify_api.test_request_context(): - mocker.patch("uuid.uuid4", return_value='some_uuid') - signed_secret = _generate_secret() - assert 'some_uuid' == _get_secret(signed_secret) - assert signed_secret != 'some_uuid' - - def test_save_api_key_should_create_new_api_key_and_history(sample_service): api_key = ApiKey(**{'service': sample_service, 'name': sample_service.name, @@ -72,13 +64,13 @@ def test_should_return_unsigned_api_keys_for_service_id(sample_api_key): unsigned_api_key = get_unsigned_secrets(sample_api_key.service_id) assert len(unsigned_api_key) == 1 assert sample_api_key.secret != unsigned_api_key[0] - assert unsigned_api_key[0] == _get_secret(sample_api_key.secret) + assert unsigned_api_key[0] == get_secret(sample_api_key.secret) def test_get_unsigned_secret_returns_key(sample_api_key): unsigned_api_key = get_unsigned_secret(sample_api_key.id) assert sample_api_key.secret != unsigned_api_key - assert unsigned_api_key == _get_secret(sample_api_key.secret) + assert unsigned_api_key == get_secret(sample_api_key.secret) def test_should_not_allow_duplicate_key_names_per_service(sample_api_key, fake_uuid): From 39519e3f36b4ace8f03e4c23148cc789434e1d01 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 29 Jun 2016 16:33:02 +0100 Subject: [PATCH 4/5] attach api_key to app we previously attached the service id and the key's secret also more refactoring of auth.py --- app/authentication/auth.py | 49 +++++++++---------- app/notifications/rest.py | 8 +-- .../app/authentication/test_authentication.py | 22 ++++++++- 3 files changed, 48 insertions(+), 31 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 193afa1cc..ab32404dc 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -2,7 +2,7 @@ from flask import request, jsonify, _request_ctx_stack, current_app from notifications_python_client.authentication import decode_jwt_token, get_token_issuer from notifications_python_client.errors import TokenDecodeError, TokenExpiredError -from app.dao.api_key_dao import get_unsigned_secrets +from app.dao.api_key_dao import get_model_api_keys def authentication_response(message, code): @@ -23,37 +23,36 @@ def requires_auth(): auth_token = auth_header[7:] try: - api_client = fetch_client(get_token_issuer(auth_token)) + client = get_token_issuer(auth_token) except TokenDecodeError: return authentication_response("Invalid token: signature", 403) - for secret in api_client['secret']: - try: - decode_jwt_token( - auth_token, - secret - ) - _request_ctx_stack.top.api_user = api_client - return - except TokenExpiredError: - errors_resp = authentication_response("Invalid token: expired", 403) - except TokenDecodeError: - errors_resp = authentication_response("Invalid token: signature", 403) + if client == current_app.config.get('ADMIN_CLIENT_USER_NAME'): + errors_resp = get_decode_errors(auth_token, current_app.config.get('ADMIN_CLIENT_SECRET'), expiry_date=None) + return errors_resp - if not api_client['secret']: + secret_keys = get_model_api_keys(client) + for api_key in secret_keys: + errors_resp = get_decode_errors(auth_token, api_key.unsigned_secret, api_key.expiry_date) + if not errors_resp: + if api_key.expiry_date: + return authentication_response("Invalid token: revoked", 403) + else: + _request_ctx_stack.top.api_user = api_key + return + + if not secret_keys: errors_resp = authentication_response("Invalid token: no api keys for service", 403) current_app.logger.info(errors_resp) return errors_resp -def fetch_client(client): - if client == current_app.config.get('ADMIN_CLIENT_USER_NAME'): - return { - "client": client, - "secret": [current_app.config.get('ADMIN_CLIENT_SECRET')] - } +def get_decode_errors(auth_token, unsigned_secret, expiry_date=None): + try: + decode_jwt_token(auth_token, unsigned_secret) + except TokenExpiredError: + return authentication_response("Invalid token: expired", 403) + except TokenDecodeError: + return authentication_response("Invalid token: signature", 403) else: - return { - "client": client, - "secret": get_unsigned_secrets(client) - } + return None diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 22186c341..cdc7fb7cb 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -169,7 +169,7 @@ def process_firetext_response(): @notifications.route('/notifications/', methods=['GET']) def get_notifications(notification_id): - notification = notifications_dao.get_notification(api_user['client'], notification_id) + notification = notifications_dao.get_notification(str(api_user.service_id), notification_id) return jsonify(data={"notification": notification_status_schema.dump(notification).data}), 200 @@ -181,7 +181,7 @@ def get_all_notifications(): limit_days = data.get('limit_days') pagination = notifications_dao.get_notifications_for_service( - api_user['client'], + str(api_user.service_id), filter_dict=data, page=page, page_size=page_size, @@ -203,8 +203,8 @@ def send_notification(notification_type): if notification_type not in ['sms', 'email']: assert False - service_id = api_user['client'] - service = services_dao.dao_fetch_service_by_id(api_user['client']) + service_id = str(api_user.service_id) + service = services_dao.dao_fetch_service_by_id(service_id) service_stats = notifications_dao.dao_get_notification_statistics_for_service_and_day( service_id, diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index 03c1d0899..daa430190 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -1,6 +1,10 @@ from datetime import datetime -from notifications_python_client.authentication import create_jwt_token + +import pytest from flask import json, current_app +from notifications_python_client.authentication import create_jwt_token + +from app import api_user from app.dao.api_key_dao import get_unsigned_secrets, save_model_api_key, get_unsigned_secret, expire_api_key from app.models import ApiKey, KEY_TYPE_NORMAL, KEY_TYPE_TEAM @@ -164,7 +168,7 @@ def test_authentication_returns_token_expired_when_service_uses_expired_key_and_ headers={'Authorization': 'Bearer {}'.format(token)}) assert response.status_code == 403 data = json.loads(response.get_data()) - assert data['message'] == {"token": ['Invalid token: signature']} + assert data['message'] == {"token": ['Invalid token: revoked']} def test_authentication_returns_error_when_api_client_has_no_secrets(notify_api, @@ -220,3 +224,17 @@ def __create_post_token(service_id, request_body): secret=get_unsigned_secrets(service_id)[0], client_id=str(service_id) ) + + +def test_should_attach_the_current_api_key_to_current_app(notify_api, sample_service, sample_api_key): + with notify_api.test_request_context() as context, notify_api.test_client() as client: + with pytest.raises(AttributeError): + print(api_user) + + token = __create_get_token(sample_api_key.service_id) + response = client.get( + '/service/{}'.format(str(sample_api_key.service_id)), + headers={'Authorization': 'Bearer {}'.format(token)} + ) + assert response.status_code == 200 + assert api_user == sample_api_key From 3838715077422695e3efc13fb6add9a105e8124b Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 29 Jun 2016 17:37:20 +0100 Subject: [PATCH 5/5] refactored the requires_auth handler to raise exceptions hopefully cleans up code flow and readability [a tiny bit]. raise an AuthException in auth.py, and catch it in errors.py to save on returning error_repsonse values throughout the function --- app/authentication/auth.py | 75 +++++++++++-------- app/errors.py | 5 ++ .../app/authentication/test_authentication.py | 6 +- 3 files changed, 51 insertions(+), 35 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index ab32404dc..8c9aac294 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -5,54 +5,65 @@ from notifications_python_client.errors import TokenDecodeError, TokenExpiredErr from app.dao.api_key_dao import get_model_api_keys -def authentication_response(message, code): - return jsonify(result='error', - message={"token": [message]} - ), code +class AuthError(Exception): + def __init__(self, message, code): + self.message = {"token": [message]} + self.code = code -def requires_auth(): - auth_header = request.headers.get('Authorization', None) +def get_auth_token(req): + auth_header = req.headers.get('Authorization', None) if not auth_header: - return authentication_response('Unauthorized, authentication token must be provided', 401) + raise AuthError('Unauthorized, authentication token must be provided', 401) auth_scheme = auth_header[:7] if auth_scheme != 'Bearer ': - return authentication_response('Unauthorized, authentication bearer scheme must be used', 401) + raise AuthError('Unauthorized, authentication bearer scheme must be used', 401) - auth_token = auth_header[7:] + return auth_header[7:] + + +def requires_auth(): + auth_token = get_auth_token(request) try: client = get_token_issuer(auth_token) except TokenDecodeError: - return authentication_response("Invalid token: signature", 403) + raise AuthError("Invalid token: signature", 403) if client == current_app.config.get('ADMIN_CLIENT_USER_NAME'): - errors_resp = get_decode_errors(auth_token, current_app.config.get('ADMIN_CLIENT_SECRET'), expiry_date=None) - return errors_resp + return handle_admin_key(auth_token, current_app.config.get('ADMIN_CLIENT_SECRET')) - secret_keys = get_model_api_keys(client) - for api_key in secret_keys: - errors_resp = get_decode_errors(auth_token, api_key.unsigned_secret, api_key.expiry_date) - if not errors_resp: - if api_key.expiry_date: - return authentication_response("Invalid token: revoked", 403) - else: - _request_ctx_stack.top.api_user = api_key - return + api_keys = get_model_api_keys(client) - if not secret_keys: - errors_resp = authentication_response("Invalid token: no api keys for service", 403) - current_app.logger.info(errors_resp) - return errors_resp + for api_key in api_keys: + try: + get_decode_errors(auth_token, api_key.unsigned_secret) + except TokenDecodeError: + continue + + if api_key.expiry_date: + raise AuthError("Invalid token: revoked", 403) + + _request_ctx_stack.top.api_user = api_key + return + + if not api_keys: + raise AuthError("Invalid token: no api keys for service", 403) + else: + raise AuthError("Invalid token: signature", 403) -def get_decode_errors(auth_token, unsigned_secret, expiry_date=None): +def handle_admin_key(auth_token, secret): + try: + get_decode_errors(auth_token, secret) + return + except TokenDecodeError as e: + raise AuthError("Invalid token: signature", 403) + + +def get_decode_errors(auth_token, unsigned_secret): try: decode_jwt_token(auth_token, unsigned_secret) - except TokenExpiredError: - return authentication_response("Invalid token: expired", 403) - except TokenDecodeError: - return authentication_response("Invalid token: signature", 403) - else: - return None + except TokenExpiredError as e: + raise AuthError("Invalid token: expired") diff --git a/app/errors.py b/app/errors.py index 9d90dee15..046f21690 100644 --- a/app/errors.py +++ b/app/errors.py @@ -5,6 +5,7 @@ from flask import ( from sqlalchemy.exc import SQLAlchemyError, DataError from sqlalchemy.orm.exc import NoResultFound from marshmallow import ValidationError +from app.authentication.auth import AuthError class InvalidRequest(Exception): @@ -23,6 +24,10 @@ class InvalidRequest(Exception): def register_errors(blueprint): + @blueprint.app_errorhandler(AuthError) + def authentication_error(error): + return jsonify(result='error', message=error.message), error.code + @blueprint.app_errorhandler(ValidationError) def validation_error(error): current_app.logger.error(error) diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index daa430190..11b54f279 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -171,9 +171,9 @@ def test_authentication_returns_token_expired_when_service_uses_expired_key_and_ assert data['message'] == {"token": ['Invalid token: revoked']} -def test_authentication_returns_error_when_api_client_has_no_secrets(notify_api, - notify_db, - notify_db_session): +def test_authentication_returns_error_when_admin_client_has_no_secrets(notify_api, + notify_db, + notify_db_session): with notify_api.test_request_context(): with notify_api.test_client() as client: api_secret = notify_api.config.get('ADMIN_CLIENT_SECRET')