diff --git a/app/main/views/api_keys.py b/app/main/views/api_keys.py index 8dbe00334..f4a3aadf0 100644 --- a/app/main/views/api_keys.py +++ b/app/main/views/api_keys.py @@ -128,7 +128,7 @@ def create_api_key(service_id): @login_required @user_has_permissions('manage_api_keys') def revoke_api_key(service_id, key_id): - key_name = api_key_api_client.get_api_keys(service_id=service_id, key_id=key_id)['apiKeys'][0]['name'] + key_name = current_service.get_api_key(key_id)['name'] if request.method == 'GET': return render_template( 'views/api/keys.html', diff --git a/app/models/service.py b/app/models/service.py index 6cf3a28f2..4b101d706 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -1,3 +1,4 @@ +from flask import abort from notifications_utils.field import Field from werkzeug.utils import cached_property @@ -344,3 +345,9 @@ class Service(): @property def api_key_names(self): return [key['name'] for key in self.api_keys] + + def get_api_key(self, id): + try: + return next(key for key in self.api_keys if key['id'] == id) + except StopIteration: + abort(404) diff --git a/app/notify_client/api_key_api_client.py b/app/notify_client/api_key_api_client.py index bfcde52b6..55bbbe500 100644 --- a/app/notify_client/api_key_api_client.py +++ b/app/notify_client/api_key_api_client.py @@ -10,11 +10,8 @@ class ApiKeyApiClient(NotifyAdminAPIClient): def __init__(self): super().__init__("a" * 73, "b") - def get_api_keys(self, service_id, key_id=None): - if key_id: - return self.get(url='/service/{}/api-keys/{}'.format(service_id, key_id)) - else: - return self.get(url='/service/{}/api-keys'.format(service_id)) + def get_api_keys(self, service_id): + return self.get(url='/service/{}/api-keys'.format(service_id)) def create_api_key(self, service_id, key_name, key_type): data = { diff --git a/tests/app/main/views/test_api_integration.py b/tests/app/main/views/test_api_integration.py index 94223c825..665a44016 100644 --- a/tests/app/main/views/test_api_integration.py +++ b/tests/app/main/views/test_api_integration.py @@ -318,16 +318,22 @@ def test_should_show_confirm_revoke_api_key( 'Confirm' ) assert mock_get_api_keys.call_args_list == [ - call( - key_id=fake_uuid, - service_id='596364a0-858e-42c8-9062-a8fe822260eb', - ), call( '596364a0-858e-42c8-9062-a8fe822260eb' ), ] +def test_should_404_for_api_key_that_doesnt_exist( + client_request, + mock_get_api_keys, +): + client_request.get( + 'main.revoke_api_key', service_id=SERVICE_ONE_ID, key_id='key-doesn’t-exist', + _expected_status=404, + ) + + def test_should_redirect_after_revoking_api_key( logged_in_client, api_user_active, @@ -338,12 +344,12 @@ def test_should_redirect_after_revoking_api_key( mock_has_permissions, fake_uuid, ): - response = logged_in_client.post(url_for('main.revoke_api_key', service_id=fake_uuid, key_id=fake_uuid)) + response = logged_in_client.post(url_for('main.revoke_api_key', service_id=SERVICE_ONE_ID, key_id=fake_uuid)) assert response.status_code == 302 - assert response.location == url_for('.api_keys', service_id=fake_uuid, _external=True) - mock_revoke_api_key.assert_called_once_with(service_id=fake_uuid, key_id=fake_uuid) - mock_get_api_keys.assert_called_once_with(service_id=fake_uuid, key_id=fake_uuid) + assert response.location == url_for('.api_keys', service_id=SERVICE_ONE_ID, _external=True) + mock_revoke_api_key.assert_called_once_with(service_id=SERVICE_ONE_ID, key_id=fake_uuid) + mock_get_api_keys.assert_called_once_with(SERVICE_ONE_ID,) @pytest.mark.parametrize('route', [ @@ -354,6 +360,7 @@ def test_should_redirect_after_revoking_api_key( def test_route_permissions( mocker, app_, + fake_uuid, api_user_active, service_one, mock_get_api_keys, @@ -365,7 +372,7 @@ def test_route_permissions( app_, "GET", 200, - url_for(route, service_id=service_one['id'], key_id=123), + url_for(route, service_id=service_one['id'], key_id=fake_uuid), ['manage_api_keys'], api_user_active, service_one) @@ -379,6 +386,7 @@ def test_route_permissions( def test_route_invalid_permissions( mocker, app_, + fake_uuid, api_user_active, service_one, mock_get_api_keys, @@ -390,7 +398,7 @@ def test_route_invalid_permissions( app_, "GET", 403, - url_for(route, service_id=service_one['id'], key_id=123), + url_for(route, service_id=service_one['id'], key_id=fake_uuid), ['view_activity'], api_user_active, service_one) diff --git a/tests/conftest.py b/tests/conftest.py index 0a451b65d..270e93151 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1639,10 +1639,12 @@ def mock_revoke_api_key(mocker): @pytest.fixture(scope='function') -def mock_get_api_keys(mocker): +def mock_get_api_keys(mocker, fake_uuid): def _get_keys(service_id, key_id=None): - keys = {'apiKeys': [api_key_json(service_id, 'some key name'), - api_key_json(service_id, 'another key name', expiry_date=str(date.fromtimestamp(0)))]} + keys = {'apiKeys': [ + api_key_json(id_=fake_uuid, name='some key name',), + api_key_json(id_='1234567', name='another key name', expiry_date=str(date.fromtimestamp(0))) + ]} return keys return mocker.patch('app.api_key_api_client.get_api_keys', side_effect=_get_keys)