From f861da184307641281e4c51ee31435b766d8e36f Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 24 Jan 2020 17:32:41 +0000 Subject: [PATCH] Improve text for error messages --- app/authentication/auth.py | 34 ++++++++++++------- app/errors.py | 4 +-- .../app/authentication/test_authentication.py | 20 +++++------ .../notifications/test_post_notifications.py | 2 +- 4 files changed, 34 insertions(+), 26 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index f8c99fb4b..9f9df9436 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -10,6 +10,9 @@ from sqlalchemy.orm.exc import NoResultFound from app.dao.services_dao import dao_fetch_service_by_id_with_api_keys +GENERAL_TOKEN_ERROR_MESSAGE = 'Invalid token: make sure your API token matches the example at https://docs.notifications.service.gov.uk/rest-api.html#authorisation-header' # noqa + + class AuthError(Exception): def __init__(self, message, code, service_id=None, api_key_id=None): self.message = {"token": [message]} @@ -36,12 +39,12 @@ class AuthError(Exception): def get_auth_token(req): auth_header = req.headers.get('Authorization', None) if not auth_header: - raise AuthError('Unauthorized, authentication token must be provided', 401) + raise AuthError('Unauthorized: authentication token must be provided', 401) auth_scheme = auth_header[:7].title() if auth_scheme != 'Bearer ': - raise AuthError('Unauthorized, authentication bearer scheme must be used', 401) + raise AuthError('Unauthorized: authentication bearer scheme must be used', 401) return auth_header[7:] @@ -60,17 +63,17 @@ def requires_admin_auth(): g.service_id = current_app.config.get('ADMIN_CLIENT_USER_NAME') return handle_admin_key(auth_token, current_app.config.get('ADMIN_CLIENT_SECRET')) else: - raise AuthError('Unauthorized, admin authentication token required', 401) + raise AuthError('Unauthorized: admin authentication token required', 401) def requires_auth(): request_helper.check_proxy_header_before_request() auth_token = get_auth_token(request) - client = __get_token_issuer(auth_token) + issuer = __get_token_issuer(auth_token) # ie the `iss` claim which should be a service ID try: - service = dao_fetch_service_by_id_with_api_keys(client) + service = dao_fetch_service_by_id_with_api_keys(issuer) except DataError: raise AuthError("Invalid token: service id is not the right data type", 403) except NoResultFound: @@ -92,10 +95,15 @@ def requires_auth(): err_msg = "Invalid token: algorithm used is not HS256" raise AuthError(err_msg, 403, service_id=service.id, api_key_id=api_key.id) except TokenDecodeError: + # we attempted to validate the token but it failed meaning it was not signed using this api key. + # Let's try the next one + # TODO: Change this so it doesn't also catch `TokenIssuerError` or `TokenIssuedAtError` exceptions (which + # are children of `TokenDecodeError`) as these should cause an auth error immediately rather than + # continue on to check the next API key continue except TokenError: - err_msg = 'Invalid token: signature, api token is not valid' - raise AuthError(err_msg, 403, service_id=service.id, api_key_id=api_key.id) + # General error when trying to decode and validate the token + raise AuthError(GENERAL_TOKEN_ERROR_MESSAGE, 403, service_id=service.id, api_key_id=api_key.id) if api_key.expiry_date: raise AuthError("Invalid token: API key revoked", 403, service_id=service.id, api_key_id=api_key.id) @@ -103,7 +111,7 @@ def requires_auth(): g.service_id = api_key.service_id _request_ctx_stack.top.authenticated_service = service _request_ctx_stack.top.api_user = api_key - current_app.logger.info('API authorised for service {} with api key {}, using client {}'.format( + current_app.logger.info('API authorised for service {} with api key {}, using issuer {}'.format( service.id, api_key.id, request.headers.get('User-Agent') @@ -111,17 +119,17 @@ def requires_auth(): return else: # service has API keys, but none matching the one the user provided - raise AuthError("Invalid token: signature, api token not found", 403, service_id=service.id) + raise AuthError("Invalid token: API key not found", 403, service_id=service.id) def __get_token_issuer(auth_token): try: - client = get_token_issuer(auth_token) + issuer = get_token_issuer(auth_token) except TokenIssuerError: raise AuthError("Invalid token: iss field not provided", 403) except TokenDecodeError: - raise AuthError("Invalid token: signature, api token is not valid", 403) - return client + raise AuthError(GENERAL_TOKEN_ERROR_MESSAGE, 403) + return issuer def handle_admin_key(auth_token, secret): @@ -130,4 +138,4 @@ def handle_admin_key(auth_token, secret): except TokenExpiredError: raise AuthError("Invalid token: expired, check that your system clock is accurate", 403) except TokenDecodeError: - raise AuthError("Invalid token: signature, api token is not valid", 403) + raise AuthError("Invalid token: could not decode your API token", 403) diff --git a/app/errors.py b/app/errors.py index 09ff8d633..a699dffc8 100644 --- a/app/errors.py +++ b/app/errors.py @@ -88,12 +88,12 @@ def register_errors(blueprint): @blueprint.errorhandler(401) def unauthorized(e): - error_message = "Unauthorized, authentication token must be provided" + error_message = "Unauthorized: authentication token must be provided" return jsonify(result='error', message=error_message), 401, [('WWW-Authenticate', 'Bearer')] @blueprint.errorhandler(403) def forbidden(e): - error_message = "Forbidden, invalid authentication token provided" + error_message = "Forbidden: invalid authentication token provided" return jsonify(result='error', message=error_message), 403 @blueprint.errorhandler(429) diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index 53c235261..b3fec11e4 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -12,7 +12,7 @@ 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 -from app.authentication.auth import AuthError, requires_admin_auth, requires_auth +from app.authentication.auth import AuthError, requires_admin_auth, requires_auth, GENERAL_TOKEN_ERROR_MESSAGE from tests.conftest import set_config @@ -22,7 +22,7 @@ def test_should_not_allow_request_with_no_token(client, auth_fn): request.headers = {} with pytest.raises(AuthError) as exc: auth_fn() - assert exc.value.short_message == 'Unauthorized, authentication token must be provided' + assert exc.value.short_message == 'Unauthorized: authentication token must be provided' @pytest.mark.parametrize('auth_fn', [requires_auth, requires_admin_auth]) @@ -30,7 +30,7 @@ def test_should_not_allow_request_with_incorrect_header(client, auth_fn): request.headers = {'Authorization': 'Basic 1234'} with pytest.raises(AuthError) as exc: auth_fn() - assert exc.value.short_message == 'Unauthorized, authentication bearer scheme must be used' + assert exc.value.short_message == 'Unauthorized: authentication bearer scheme must be used' @pytest.mark.parametrize('auth_fn', [requires_auth, requires_admin_auth]) @@ -38,7 +38,7 @@ def test_should_not_allow_request_with_incorrect_token(client, auth_fn): request.headers = {'Authorization': 'Bearer 1234'} with pytest.raises(AuthError) as exc: auth_fn() - assert exc.value.short_message == 'Invalid token: signature, api token is not valid' + assert exc.value.short_message == GENERAL_TOKEN_ERROR_MESSAGE @pytest.mark.parametrize('auth_fn', [requires_auth, requires_admin_auth]) @@ -80,7 +80,7 @@ def test_auth_should_not_allow_request_with_no_iat(client, sample_api_key): request.headers = {'Authorization': 'Bearer {}'.format(token)} with pytest.raises(AuthError) as exc: requires_auth() - assert exc.value.short_message == 'Invalid token: signature, api token not found' + assert exc.value.short_message == 'Invalid token: API key not found' def test_auth_should_not_allow_request_with_non_hs256_algorithm(client, sample_api_key): @@ -123,7 +123,7 @@ def test_admin_auth_should_not_allow_request_with_no_iat(client, sample_api_key) request.headers = {'Authorization': 'Bearer {}'.format(token)} with pytest.raises(AuthError) as exc: requires_admin_auth() - assert exc.value.short_message == 'Invalid token: signature, api token is not valid' + assert exc.value.short_message == "Invalid token: could not decode your API token" def test_auth_should_not_allow_request_with_extra_claims(client, sample_api_key): @@ -146,7 +146,7 @@ def test_auth_should_not_allow_request_with_extra_claims(client, sample_api_key) request.headers = {'Authorization': 'Bearer {}'.format(token)} with pytest.raises(AuthError) as exc: requires_auth() - assert exc.value.short_message == 'Invalid token: signature, api token is not valid' + assert exc.value.short_message == GENERAL_TOKEN_ERROR_MESSAGE def test_should_not_allow_invalid_secret(client, sample_api_key): @@ -159,7 +159,7 @@ def test_should_not_allow_invalid_secret(client, sample_api_key): ) assert response.status_code == 403 data = json.loads(response.get_data()) - assert data['message'] == {"token": ['Invalid token: signature, api token not found']} + assert data['message'] == {"token": ['Invalid token: API key not found']} @pytest.mark.parametrize('scheme', ['bearer', 'Bearer']) @@ -276,7 +276,7 @@ def test_authentication_returns_error_when_admin_client_has_no_secrets(client): headers={'Authorization': 'Bearer {}'.format(token)}) assert response.status_code == 403 error_message = json.loads(response.get_data()) - assert error_message['message'] == {"token": ["Invalid token: signature, api token is not valid"]} + assert error_message['message'] == {"token": ["Invalid token: could not decode your API token"]} def test_authentication_returns_error_when_admin_client_secret_is_invalid(client): @@ -291,7 +291,7 @@ def test_authentication_returns_error_when_admin_client_secret_is_invalid(client headers={'Authorization': 'Bearer {}'.format(token)}) assert response.status_code == 403 error_message = json.loads(response.get_data()) - assert error_message['message'] == {"token": ["Invalid token: signature, api token is not valid"]} + assert error_message['message'] == {"token": ["Invalid token: could not decode your API token"]} current_app.config['ADMIN_CLIENT_SECRET'] = api_secret diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 53b62d730..02cab3c15 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -256,7 +256,7 @@ def test_post_notification_returns_401_and_well_formed_auth_error(client, sample error_resp = json.loads(response.get_data(as_text=True)) assert error_resp['status_code'] == 401 assert error_resp['errors'] == [{'error': "AuthError", - 'message': 'Unauthorized, authentication token must be provided'}] + 'message': 'Unauthorized: authentication token must be provided'}] @pytest.mark.parametrize("notification_type, key_send_to, send_to",