From adbe02783d0ee1b32e6d12eafa911055fdb0c756 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 29 Jun 2016 14:15:32 +0100 Subject: [PATCH] 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):