diff --git a/app/main/forms.py b/app/main/forms.py index c347d714e..2d94a31f1 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -525,9 +525,11 @@ class ChooseTimeForm(StripWhitespaceForm): class CreateKeyForm(StripWhitespaceForm): - def __init__(self, existing_key_names=[], *args, **kwargs): - self.existing_key_names = [x.lower() for x in existing_key_names] - super(CreateKeyForm, self).__init__(*args, **kwargs) + def __init__(self, existing_keys, *args, **kwargs): + self.existing_key_names = [ + key['name'].lower() for key in existing_keys + ] + super().__init__(*args, **kwargs) key_type = RadioField( 'Type of key', diff --git a/app/main/views/api_keys.py b/app/main/views/api_keys.py index 95048eabf..e59aaf770 100644 --- a/app/main/views/api_keys.py +++ b/app/main/views/api_keys.py @@ -80,7 +80,6 @@ def whitelist(service_id): def api_keys(service_id): return render_template( 'views/api/keys.html', - keys=api_key_api_client.get_api_keys(service_id=service_id)['apiKeys'] ) @@ -88,10 +87,7 @@ def api_keys(service_id): @login_required @user_has_permissions('manage_api_keys', restrict_admin_usage=True) def create_api_key(service_id): - key_names = [ - key['name'] for key in api_key_api_client.get_api_keys(service_id=service_id)['apiKeys'] - ] - form = CreateKeyForm(key_names) + form = CreateKeyForm(current_service.api_keys) form.key_type.choices = [ (KEY_TYPE_NORMAL, 'Live – sends to anyone'), (KEY_TYPE_TEAM, 'Team and whitelist – limits who you can send to'), @@ -132,12 +128,11 @@ 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', revoke_key=key_name, - keys=api_key_api_client.get_api_keys(service_id=service_id)['apiKeys'], ) elif request.method == 'POST': api_key_api_client.revoke_api_key(service_id=service_id, key_id=key_id) diff --git a/app/models/service.py b/app/models/service.py index ac00a3e5d..2ff9ad42e 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -2,6 +2,7 @@ from flask import abort from notifications_utils.field import Field from werkzeug.utils import cached_property +from app.notify_client.api_key_api_client import api_key_api_client from app.notify_client.billing_api_client import billing_api_client from app.notify_client.email_branding_client import email_branding_client from app.notify_client.inbound_number_client import inbound_number_client @@ -364,3 +365,16 @@ class Service(): template_ids=ids_to_move & self.all_template_ids, folder_ids=ids_to_move & self.all_template_folder_ids, ) + + @cached_property + def api_keys(self): + return sorted( + api_key_api_client.get_api_keys(self.id)['apiKeys'], + key=lambda key: key['name'].lower(), + ) + + 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/app/templates/views/api/keys.html b/app/templates/views/api/keys.html index 7e53cd4ab..5bbfc0e28 100644 --- a/app/templates/views/api/keys.html +++ b/app/templates/views/api/keys.html @@ -37,7 +37,7 @@
{% call(item, row_number) list_table( - keys, + current_service.api_keys, empty_message="You haven’t created any API keys yet", caption="API keys", caption_visible=false, diff --git a/tests/app/main/test_create_api_key_form.py b/tests/app/main/test_create_api_key_form.py index c302093d6..374a0c46a 100644 --- a/tests/app/main/test_create_api_key_form.py +++ b/tests/app/main/test_create_api_key_form.py @@ -5,10 +5,12 @@ from app.main.forms import CreateKeyForm def test_return_validation_error_when_key_name_exists(client): - def _get_names(): - return ['some key', 'another key'] + _existing_keys = [ + {'name': 'some key'}, + {'name': 'another key'}, + ] - form = CreateKeyForm(_get_names(), + form = CreateKeyForm(_existing_keys, formdata=MultiDict([('key_name', 'Some key')])) form.key_type.choices = [('a', 'a'), ('b', 'b')] form.validate() diff --git a/tests/app/main/views/test_api_integration.py b/tests/app/main/views/test_api_integration.py index e393e3c44..a0e62694a 100644 --- a/tests/app/main/views/test_api_integration.py +++ b/tests/app/main/views/test_api_integration.py @@ -186,26 +186,21 @@ def test_should_show_empty_api_keys_page( assert response.status_code == 200 assert 'You haven’t created any API keys yet' in response.get_data(as_text=True) assert 'Create an API key' in response.get_data(as_text=True) - mock_get_no_api_keys.assert_called_once_with(service_id=service_id) + mock_get_no_api_keys.assert_called_once_with(service_id) def test_should_show_api_keys_page( - logged_in_client, - api_user_active, - mock_login, + client_request, mock_get_api_keys, - mock_get_service, - mock_has_permissions, - fake_uuid, ): - response = logged_in_client.get(url_for('main.api_keys', service_id=fake_uuid)) + page = client_request.get('main.api_keys', service_id=SERVICE_ONE_ID) + rows = [normalize_spaces(row.text) for row in page.select('main tr')] - assert response.status_code == 200 - resp_data = response.get_data(as_text=True) - assert 'some key name' in resp_data - assert 'another key name' in resp_data - assert 'Revoked 1 January at 1:00am' in resp_data - mock_get_api_keys.assert_called_once_with(service_id=fake_uuid) + assert rows[0] == 'API keys Action' + assert rows[1] == 'another key name Revoked 1 January at 1:00am' + assert rows[2] == 'some key name Revoke' + + mock_get_api_keys.assert_called_once_with(SERVICE_ONE_ID) @pytest.mark.parametrize('service_mock, expected_options', [ @@ -319,15 +314,21 @@ def test_should_show_confirm_revoke_api_key( ) assert mock_get_api_keys.call_args_list == [ call( - key_id=fake_uuid, - service_id='596364a0-858e-42c8-9062-a8fe822260eb', - ), - call( - service_id='596364a0-858e-42c8-9062-a8fe822260eb' + '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 +339,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 +355,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 +367,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 +381,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 +393,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)