wip

app/authentication/auth.py

@@ -1,7 +1,7 @@
 from flask import request, _request_ctx_stack, current_app, g
 from notifications_python_client.authentication import decode_jwt_token, get_token_issuer
 from notifications_python_client.errors import (
-    TokenDecodeError, TokenExpiredError, TokenIssuerError, TokenAlgorithmError
+    TokenDecodeError, TokenExpiredError, TokenIssuerError, TokenAlgorithmError, TokenError
 )
 from notifications_utils import request_helper
 from sqlalchemy.exc import DataError
@@ -9,6 +9,8 @@ from sqlalchemy.orm.exc import NoResultFound
 
 from app.dao.services_dao import dao_fetch_service_by_id_with_api_keys
 
+TOKEN_ERROR_GUIDANCE = "See our requirements for JSON Web Tokens 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):
@@ -36,12 +38,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 +62,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:
@@ -85,14 +87,23 @@ def requires_auth():
     for api_key in service.api_keys:
         try:
             decode_jwt_token(auth_token, api_key.secret)
-        except TokenExpiredError:
-            err_msg = "Error: Your system clock must be accurate to within 30 seconds"
-            raise AuthError(err_msg, 403, service_id=service.id, api_key_id=api_key.id)
         except TokenAlgorithmError:
-            err_msg = "Invalid token: algorithm used is not HS256"
+            # During decoding and validation, it appears the token was created with an algorithm
+            # we don't allow so it fails validation
+            err_msg = "Invalid token: algorithm used is not HS256. " + TOKEN_ERROR_GUIDANCE
             raise AuthError(err_msg, 403, service_id=service.id, api_key_id=api_key.id)
         except TokenDecodeError:
+            # Given the algorithm chosen was fine, we attempted to validate the token but it failed
+            # meaning it was not signed using this api key, let's try the next one
             continue
+        except TokenExpiredError:
+            # API key matches for this service but there was an error with the expiry of the token
+            err_msg = "Error: Your system clock must be accurate to within 30 seconds"
+            raise AuthError(err_msg, 403, service_id=service.id, api_key_id=api_key.id)
+        except TokenError:
+            err_msg = "Invalid token: API token is not valid. " + TOKEN_ERROR_GUIDANCE
+            raise AuthError(err_msg, 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)
@@ -100,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')
@@ -108,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)
+        raise AuthError("Invalid token: iss field not provided. " + TOKEN_ERROR_GUIDANCE, 403)
     except TokenDecodeError:
-        raise AuthError("Invalid token: signature, api token is not valid", 403)
+        raise AuthError("Invalid token: API token is not valid. " + TOKEN_ERROR_GUIDANCE, 403)
-    return client
+    return issuer
 
 
 def handle_admin_key(auth_token, secret):
@@ -127,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: API token is not valid", 403)

requirements-app.txt

@@ -20,7 +20,7 @@ psycopg2-binary==2.8.4
 PyJWT==1.7.1
 SQLAlchemy==1.3.10
 
-notifications-python-client==5.4.1
+notifications-python-client==5.5.0
 
 # PaaS
 awscli-cwlogs>=1.4,<1.5

requirements.txt

@@ -22,7 +22,7 @@ psycopg2-binary==2.8.4
 PyJWT==1.7.1
 SQLAlchemy==1.3.10
 
-notifications-python-client==5.4.1
+notifications-python-client==5.5.0
 
 # PaaS
 awscli-cwlogs>=1.4,<1.5
@@ -40,12 +40,12 @@ alembic==1.3.2
 amqp==1.4.9
 anyjson==0.3.3
 attrs==19.3.0
-awscli==1.16.310
+awscli==1.16.313
 bcrypt==3.1.7
 billiard==3.3.0.23
 bleach==3.1.0
 boto3==1.10.38
-botocore==1.13.46
+botocore==1.13.49
 certifi==2019.11.28
 chardet==3.0.4
 Click==7.0
@@ -70,7 +70,7 @@ phonenumbers==8.11.1
 pyasn1==0.4.8
 pycparser==2.19
 PyPDF2==1.26.0
-pyrsistent==0.15.6
+pyrsistent==0.15.7
 python-dateutil==2.8.1
 python-editor==1.0.4
 python-json-logger==0.1.11

tests/app/authentication/test_authentication.py

@@ -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 'Invalid token: API token is not valid. See our requirements' in exc.value.short_message
 
 
 @pytest.mark.parametrize('auth_fn', [requires_auth, requires_admin_auth])
@@ -59,7 +59,7 @@ def test_should_not_allow_request_with_no_iss(client, auth_fn):
     request.headers = {'Authorization': 'Bearer {}'.format(token)}
     with pytest.raises(AuthError) as exc:
         auth_fn()
-    assert exc.value.short_message == 'Invalid token: iss field not provided'
+    assert 'Invalid token: iss field not provided. See our requirements' in exc.value.short_message
 
 
 def test_auth_should_not_allow_request_with_no_iat(client, sample_api_key):
@@ -80,28 +80,8 @@ 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'
+    # TODO: This is not the correct error message to show here
-
+    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):
-    iss = str(sample_api_key.service_id)
-    # code copied from notifications_python_client.authentication.py::create_jwt_token
-    headers = {
-        "typ": 'JWT',
-        "alg": 'HS512'
-    }
-
-    claims = {
-        'iss': iss,
-        'iat': int(time.time())
-    }
-
-    token = jwt.encode(payload=claims, key=str(uuid.uuid4()), headers=headers).decode()
-
-    request.headers = {'Authorization': 'Bearer {}'.format(token)}
-    with pytest.raises(AuthError) as exc:
-        requires_auth()
-    assert exc.value.short_message == 'Invalid token: algorithm used is not HS256'
 
 
 def test_admin_auth_should_not_allow_request_with_no_iat(client, sample_api_key):
@@ -123,7 +103,52 @@ 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: API token is not valid'
+
+
+def test_auth_should_not_allow_request_with_non_hs256_algorithm(client, sample_api_key):
+    iss = str(sample_api_key.service_id)
+    # code copied from notifications_python_client.authentication.py::create_jwt_token
+    headers = {
+        "typ": 'JWT',
+        "alg": 'HS512'
+    }
+
+    claims = {
+        'iss': iss,
+        'iat': int(time.time())
+    }
+
+    token = jwt.encode(payload=claims, key=str(uuid.uuid4()), headers=headers).decode()
+
+    request.headers = {'Authorization': 'Bearer {}'.format(token)}
+    with pytest.raises(AuthError) as exc:
+        requires_auth()
+    assert 'Invalid token: algorithm used is not HS256. See our requirements' in exc.value.short_message
+
+
+def test_auth_should_not_allow_request_with_extra_claims(client, sample_api_key):
+    iss = str(sample_api_key.service_id)
+    key = get_unsigned_secrets(sample_api_key.service_id)[0]
+    # code copied from notifications_python_client.authentication.py::create_jwt_token
+    headers = {
+        "typ": 'JWT',
+        "alg": 'HS256'
+    }
+
+    claims = {
+        'iss': iss,
+        'iat': int(time.time()),
+        'aud': 'notifications.service.gov.uk'  # extra claim that we don't support
+    }
+
+    token = jwt.encode(payload=claims, key=key, headers=headers).decode()
+
+    request.headers = {'Authorization': 'Bearer {}'.format(token)}
+    with pytest.raises(AuthError) as exc:
+        requires_auth()
+    # TODO: this is not the correct error message to show here
+    assert exc.value.short_message == 'Invalid token: API key not found'
 
 
 def test_should_not_allow_invalid_secret(client, sample_api_key):
@@ -136,7 +161,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'])
@@ -253,7 +278,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: API token is not valid"]}
 
 
 def test_authentication_returns_error_when_admin_client_secret_is_invalid(client):
@@ -268,7 +293,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: API token is not valid"]}
     current_app.config['ADMIN_CLIENT_SECRET'] = api_secret