Standardise auth checks for both kinds of API auth

Previously "requires_auth" and "requires_admin_auth" had similar
but different ways of checking their keys. This switches them to
use the same checks, with the admin / internal auth passing in a
fake / stub set of "api keys" to check.

Pulling out the logic this way will make it easier to unpick the
tests, so we can focus on testing what's unique to each kind of
API auth and avoid future duplication when we start calling the
"requires_internal_auth" method with other client_ids.

Note that a couple of error messages / response codes have changed
for admin / internal auth. None of these occur in practice, so we
can make them consistent with the behaviour for the public API.

app/authentication/auth.py

@@ -49,6 +49,13 @@ class AuthError(Exception):
         }
 
 
+class InternalApiKey():
+    def __init__(self, client_id, secret):
+        self.secret = secret
+        self.id = client_id
+        self.expiry_date = None
+
+
 def get_auth_token(req):
     auth_header = req.headers.get('Authorization', None)
     if not auth_header:
@@ -81,23 +88,13 @@ def requires_internal_auth(expected_client_id):
     if client_id != expected_client_id:
         raise AuthError("Unauthorized: not allowed to perform this action", 401)
 
+    api_keys = [
+        InternalApiKey(client_id, secret)
+        for secret in current_app.config.get('INTERNAL_CLIENT_API_KEYS')[client_id]
+    ]
+
+    _decode_jwt_token(auth_token, api_keys, client_id)
     g.service_id = client_id
-    secrets = current_app.config.get('INTERNAL_CLIENT_API_KEYS')[client_id]
-
-    for secret in secrets:
-        try:
-            decode_jwt_token(auth_token, secret)
-            return
-        except TokenExpiredError:
-            raise AuthError("Invalid token: expired, check that your system clock is accurate", 403)
-        except TokenDecodeError:
-            # 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 admin client secret
-            continue
-
-    # Either there are no admin client secrets or their token didn't match one of them so error
-    raise AuthError("Unauthorized: API authentication token not found", 401)
 
 
 def requires_auth():
@@ -123,15 +120,23 @@ def requires_auth():
     if not service.active:
         raise AuthError("Invalid token: service is archived", 403, service_id=service.id)
 
-    for api_key in service.api_keys:
+    api_key = _decode_jwt_token(auth_token, service.api_keys, service.id)
+
+    g.api_user = api_key
+    g.service_id = service_id
+    g.authenticated_service = service
+
+
+def _decode_jwt_token(auth_token, api_keys, service_id=None):
+    for api_key in 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)
+            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"
-            raise AuthError(err_msg, 403, service_id=service.id, api_key_id=api_key.id)
+            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
@@ -141,25 +146,22 @@ def requires_auth():
             continue
         except TokenError:
             # 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)
+            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)
-
-        g.service_id = service.id
-        g.api_user = api_key
-        g.authenticated_service = service
+            raise AuthError("Invalid token: API key revoked", 403, service_id=service_id, api_key_id=api_key.id)
 
         current_app.logger.info('API authorised for service {} with api key {}, using issuer {} for URL: {}'.format(
-            service.id,
+            service_id,
             api_key.id,
             request.headers.get('User-Agent'),
             request.base_url
         ))
-        return
+
+        return api_key
     else:
         # service has API keys, but none matching the one the user provided
-        raise AuthError("Invalid token: API key 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):

tests/app/authentication/test_authentication.py

@@ -134,7 +134,7 @@ def test_requires_admin_auth_should_not_allow_request_with_no_iat(
     request.headers = {'Authorization': 'Bearer {}'.format(token)}
     with pytest.raises(AuthError) as exc:
         requires_admin_auth()
-    assert exc.value.short_message == "Unauthorized: API authentication token not found"
+    assert exc.value.short_message == "Invalid token: API key not found"
 
 
 def test_requires_admin_auth_should_not_allow_request_with_old_iat(
@@ -150,7 +150,7 @@ def test_requires_admin_auth_should_not_allow_request_with_old_iat(
     request.headers = {'Authorization': 'Bearer {}'.format(token)}
     with pytest.raises(AuthError) as exc:
         requires_admin_auth()
-    assert exc.value.short_message == "Invalid token: expired, check that your system clock is accurate"
+    assert exc.value.short_message == "Error: Your system clock must be accurate to within 30 seconds"
 
 
 def test_requires_auth_should_not_allow_request_with_extra_claims(
@@ -342,9 +342,9 @@ def test_requires_admin_auth_returns_error_with_no_secrets(
             '/service',
             headers={'Authorization': 'Bearer {}'.format(admin_jwt_token)})
 
-    assert response.status_code == 401
+    assert response.status_code == 403
     error_message = json.loads(response.get_data())
-    assert error_message['message'] == {"token": ["Unauthorized: API authentication token not found"]}
+    assert error_message['message'] == {"token": ["Invalid token: API key not found"]}
 
 
 def test_requires_admin_auth_returns_error_when_secret_is_invalid(
@@ -359,9 +359,9 @@ def test_requires_admin_auth_returns_error_when_secret_is_invalid(
             '/service',
             headers={'Authorization': 'Bearer {}'.format(admin_jwt_token)})
 
-    assert response.status_code == 401
+    assert response.status_code == 403
     error_message = json.loads(response.get_data())
-    assert error_message['message'] == {"token": ["Unauthorized: API authentication token not found"]}
+    assert error_message['message'] == {"token": ["Invalid token: API key not found"]}
 
 
 def test_requires_auth_returns_error_when_service_doesnt_exist(