From d1c9dcfb1d6763aff7a2a5d5df99e1e142a18dc2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 7 Nov 2018 11:35:24 +0000 Subject: [PATCH 1/5] Put API keys on service model Similar to how we put templates on the service model, it means less logic needs to happen in the view code. --- app/main/views/api_keys.py | 7 +------ app/models/service.py | 9 +++++++++ app/templates/views/api/keys.html | 2 +- tests/app/main/views/test_api_integration.py | 6 +++--- 4 files changed, 14 insertions(+), 10 deletions(-) diff --git a/app/main/views/api_keys.py b/app/main/views/api_keys.py index 95048eabf..8dbe00334 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_key_names) form.key_type.choices = [ (KEY_TYPE_NORMAL, 'Live – sends to anyone'), (KEY_TYPE_TEAM, 'Team and whitelist – limits who you can send to'), @@ -137,7 +133,6 @@ def revoke_api_key(service_id, key_id): 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 7780347f4..6cf3a28f2 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -1,6 +1,7 @@ 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 @@ -335,3 +336,11 @@ 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 api_key_api_client.get_api_keys(self.id)['apiKeys'] + + @property + def api_key_names(self): + return [key['name'] for key in self.api_keys] 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/views/test_api_integration.py b/tests/app/main/views/test_api_integration.py index e393e3c44..94223c825 100644 --- a/tests/app/main/views/test_api_integration.py +++ b/tests/app/main/views/test_api_integration.py @@ -186,7 +186,7 @@ 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( @@ -205,7 +205,7 @@ def test_should_show_api_keys_page( 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) + mock_get_api_keys.assert_called_once_with(fake_uuid) @pytest.mark.parametrize('service_mock, expected_options', [ @@ -323,7 +323,7 @@ def test_should_show_confirm_revoke_api_key( service_id='596364a0-858e-42c8-9062-a8fe822260eb', ), call( - service_id='596364a0-858e-42c8-9062-a8fe822260eb' + '596364a0-858e-42c8-9062-a8fe822260eb' ), ] From 591bbe9a4994f1c707a860e68f5b0e33003417b9 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 7 Nov 2018 11:53:29 +0000 Subject: [PATCH 2/5] =?UTF-8?q?Don=E2=80=99t=20do=20multiple=20get=20API?= =?UTF-8?q?=20calls=20when=20revoking?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s redundant to make two API calls here, one to get all keys and one to get a single key. Since the API calls are sequential we can speed things up by getting the one key from the list of all keys. --- app/main/views/api_keys.py | 2 +- app/models/service.py | 7 +++++ app/notify_client/api_key_api_client.py | 7 ++--- tests/app/main/views/test_api_integration.py | 28 +++++++++++++------- tests/conftest.py | 8 +++--- 5 files changed, 33 insertions(+), 19 deletions(-) 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) From 2639e2c46f44cfe99922ea8076cd8fb746c0dd12 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 7 Nov 2018 12:05:11 +0000 Subject: [PATCH 3/5] Sort API keys alphabetically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently the order of API keys seems to be non-deterministic: https://github.com/alphagov/notifications-api/blob/d46caa184e3e91903829336788c96ffeb2baa57d/app/dao/api_key_dao.py#L32-L39 Generally we sort things alphabetically, unless there’s a good reason to do otherwise. This commit sorts the API keys alphabetically. --- app/models/service.py | 5 ++++- tests/app/main/views/test_api_integration.py | 21 ++++++++------------ 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/app/models/service.py b/app/models/service.py index 4b101d706..731b5fda6 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -340,7 +340,10 @@ class Service(): @cached_property def api_keys(self): - return api_key_api_client.get_api_keys(self.id)['apiKeys'] + return sorted( + api_key_api_client.get_api_keys(self.id)['apiKeys'], + key=lambda key: key['name'].lower(), + ) @property def api_key_names(self): diff --git a/tests/app/main/views/test_api_integration.py b/tests/app/main/views/test_api_integration.py index 665a44016..a0e62694a 100644 --- a/tests/app/main/views/test_api_integration.py +++ b/tests/app/main/views/test_api_integration.py @@ -190,22 +190,17 @@ def test_should_show_empty_api_keys_page( 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(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', [ From 8bedcb5af80f9f44816a7615cba6d9aa3842acbd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 13 Nov 2018 09:57:17 +0000 Subject: [PATCH 4/5] Move data transformation into the form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follows what we’re doing with the folders stuff. Avoids having too many very straightforward methods on the model. Especially when the data they need is only used by the form. So it’s better to encapsulate the logic in the form. --- app/main/forms.py | 8 +++++--- app/main/views/api_keys.py | 2 +- app/models/service.py | 4 ---- tests/app/main/test_create_api_key_form.py | 8 +++++--- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index c347d714e..e84a3d2b6 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 f4a3aadf0..e59aaf770 100644 --- a/app/main/views/api_keys.py +++ b/app/main/views/api_keys.py @@ -87,7 +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): - form = CreateKeyForm(current_service.api_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'), diff --git a/app/models/service.py b/app/models/service.py index 731b5fda6..4cdc12360 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -345,10 +345,6 @@ class Service(): key=lambda key: key['name'].lower(), ) - @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) 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() From 872b3f9c94c53e7a21250bb718721ffcdc6e70f9 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 13 Nov 2018 14:27:01 +0000 Subject: [PATCH 5/5] Make existing_keys a required argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because we should always expect its presence, even it’s an empty list. And because having mutable default arguments can have nasty side effects. --- app/main/forms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/main/forms.py b/app/main/forms.py index e84a3d2b6..2d94a31f1 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -525,7 +525,7 @@ class ChooseTimeForm(StripWhitespaceForm): class CreateKeyForm(StripWhitespaceForm): - def __init__(self, existing_keys=[], *args, **kwargs): + def __init__(self, existing_keys, *args, **kwargs): self.existing_key_names = [ key['name'].lower() for key in existing_keys ]