From 6b3c89912714e23260e74fa35e2eef0478910333 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 16 Sep 2016 08:36:44 +0100 Subject: [PATCH 1/3] Add a test for auth-ing with non-existant service MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you create a token signed with a service ID that doesn’t exist, you will get an error (as you should). However we didn’t have a test that explicitly checks for this. This commit adds one. --- .../app/authentication/test_authentication.py | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index bae0b1e03..0fb409dd2 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -192,6 +192,28 @@ def test_authentication_returns_error_when_admin_client_has_no_secrets(notify_ap notify_api.config['ADMIN_CLIENT_SECRET'] = api_secret +def test_authentication_returns_error_when_service_doesnt_exit( + notify_api, + notify_db, + notify_db_session, + sample_service, + fake_uuid +): + with notify_api.test_request_context(), notify_api.test_client() as client: + # get service ID and secret the wrong way around + token = create_jwt_token( + secret=str(sample_service.id), + client_id=fake_uuid + ) + response = client.get( + '/service', + headers={'Authorization': 'Bearer {}'.format(token)} + ) + assert response.status_code == 403 + error_message = json.loads(response.get_data()) + assert error_message['message'] == {'token': ['Invalid token: no api keys for service']} + + def test_authentication_returns_error_when_service_has_no_secrets(notify_api, notify_db, notify_db_session, From 1ce91997e8faa99a7e9e0405969a965fb290065e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 16 Sep 2016 08:44:08 +0100 Subject: [PATCH 2/3] =?UTF-8?q?Give=20specifc=20error=20when=20service=20d?= =?UTF-8?q?oesn=E2=80=99t=20exist?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you sign a token with a service ID that doesn’t exist (say, for example, that you get service ID and API key mixed up) then you get an error saying that “no API keys exist for the service”. This is wrong because the service doesn’t even exist. This commit adds: - code to check if the service does exist - a specific error message for this case The check does mean an extra database call to look up the service. However this only happens _after_ looping through all the API keys. So it shouldn’t have a performance implication for anyone using a valid API key. --- app/authentication/auth.py | 8 ++++++++ tests/app/authentication/test_authentication.py | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 5353c4e4f..d7f842aab 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -1,8 +1,11 @@ from flask import request, jsonify, _request_ctx_stack, current_app +from sqlalchemy.orm.exc import NoResultFound + 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_model_api_keys +from app.dao.services_dao import dao_fetch_service_by_id class AuthError(Exception): @@ -48,6 +51,11 @@ def requires_auth(): _request_ctx_stack.top.api_user = api_key return + try: + dao_fetch_service_by_id(client) + except NoResultFound: + raise AuthError("Invalid token: service not found", 403) + if not api_keys: raise AuthError("Invalid token: no api keys for service", 403) else: diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index 0fb409dd2..7fdc1b922 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -211,7 +211,7 @@ def test_authentication_returns_error_when_service_doesnt_exit( ) assert response.status_code == 403 error_message = json.loads(response.get_data()) - assert error_message['message'] == {'token': ['Invalid token: no api keys for service']} + assert error_message['message'] == {'token': ['Invalid token: service not found']} def test_authentication_returns_error_when_service_has_no_secrets(notify_api, From d44a0b72bb1668a236151a00f81124c15d7e9f22 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 16 Sep 2016 08:45:48 +0100 Subject: [PATCH 3/3] Rewrite authentication error messages more English --- app/authentication/auth.py | 4 ++-- tests/app/authentication/test_authentication.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index d7f842aab..5e8cb2d4e 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -46,7 +46,7 @@ def requires_auth(): continue if api_key.expiry_date: - raise AuthError("Invalid token: revoked", 403) + raise AuthError("Invalid token: API key revoked", 403) _request_ctx_stack.top.api_user = api_key return @@ -57,7 +57,7 @@ def requires_auth(): raise AuthError("Invalid token: service not found", 403) if not api_keys: - raise AuthError("Invalid token: no api keys for service", 403) + raise AuthError("Invalid token: service has no API keys", 403) else: raise AuthError("Invalid token: signature", 403) diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index 7fdc1b922..057760585 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -169,7 +169,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: revoked']} + assert data['message'] == {"token": ['Invalid token: API key revoked']} def test_authentication_returns_error_when_admin_client_has_no_secrets(notify_api, @@ -230,7 +230,7 @@ def test_authentication_returns_error_when_service_has_no_secrets(notify_api, headers={'Authorization': 'Bearer {}'.format(token)}) assert response.status_code == 403 error_message = json.loads(response.get_data()) - assert error_message['message'] == {'token': ['Invalid token: no api keys for service']} + assert error_message['message'] == {'token': ['Invalid token: service has no API keys']} def test_should_attach_the_current_api_key_to_current_app(notify_api, sample_service, sample_api_key):