From 4ddc99781f9979473dbbed86734111900a0121db Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 7 Oct 2016 10:59:07 +0100 Subject: [PATCH 1/2] Make revoked message in API keys table lighter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s secondary information so works better in the secondary text colour. --- app/templates/views/api/keys.html | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/templates/views/api/keys.html b/app/templates/views/api/keys.html index a18419156..a512dd572 100644 --- a/app/templates/views/api/keys.html +++ b/app/templates/views/api/keys.html @@ -19,7 +19,6 @@ Create an API key - {% call(item, row_number) list_table( keys, empty_message="You haven’t created any API keys yet", @@ -47,7 +46,7 @@ {% endcall %} {% if item.expiry_date %} {% call field(align='right') %} - Revoked {{ item.expiry_date|format_datetime_short }} + Revoked {{ item.expiry_date|format_datetime_short }} {% endcall %} {% else %} {% call field(align='right', status='error') %} From 6946d3af545dc5d20d48f3349ee08993807e3b35 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 7 Oct 2016 10:59:32 +0100 Subject: [PATCH 2/2] Make API key combination of secret and service ID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In research we’ve seen people mix up the service ID and API key because they’re both 36 character UUIDs. We can’t get rid of the service ID because it’s used to look up the API key. Instead, we should change API key to be one long string, which contains both the service ID, API key and (optionally) the name of the key. For example: ``` casework_production-8b3aa916-ec82-434e-b0c5-d5d9b371d6a3-dcdc5083-2fee-4fba-8afd-51f3f4bcb7b0 ``` We still need to keep the old, separate, key and service ID for a while until people have updated their clients. But they’re now both on this page, rather than on two separate pages, which should make for less fussing anyway. This shouldn’t be rolled out until the new clients are available. - [ ] https://github.com/alphagov/notifications-python-client/pull/36 - [ ] https://github.com/alphagov/notifications-node-client/pull/10 - [ ] https://github.com/alphagov/notifications-ruby-client/pull/15 - [ ] https://github.com/alphagov/notifications-java-client/pull/38 - [ ] PHP???? --- app/main/views/api_keys.py | 10 ++++++--- app/templates/views/api/keys.html | 4 ---- app/templates/views/api/keys/show.html | 30 +++++++++++++++++++++----- app/utils.py | 6 +++--- tests/app/main/views/test_api_keys.py | 14 +++++++++--- 5 files changed, 46 insertions(+), 18 deletions(-) diff --git a/app/main/views/api_keys.py b/app/main/views/api_keys.py index ace3da731..de7c65181 100644 --- a/app/main/views/api_keys.py +++ b/app/main/views/api_keys.py @@ -3,7 +3,7 @@ from flask_login import login_required from app.main import main from app.main.forms import CreateKeyForm, Whitelist from app import api_key_api_client, service_api_client, notification_api_client, current_service -from app.utils import user_has_permissions +from app.utils import user_has_permissions, email_safe from app.notify_client.api_key_api_client import KEY_TYPE_NORMAL, KEY_TYPE_TEST, KEY_TYPE_TEAM @@ -80,8 +80,12 @@ def create_api_key(service_id): key_name=form.key_name.data, key_type=form.key_type.data ) - return render_template('views/api/keys/show.html', secret=secret, - key_name=form.key_name.data) + return render_template( + 'views/api/keys/show.html', + secret=secret, + service_id=service_id, + key_name=email_safe(form.key_name.data, whitespace='_') + ) return render_template( 'views/api/keys/create.html', form=form diff --git a/app/templates/views/api/keys.html b/app/templates/views/api/keys.html index a512dd572..5c67da32f 100644 --- a/app/templates/views/api/keys.html +++ b/app/templates/views/api/keys.html @@ -55,10 +55,6 @@ {% endif %} {% endcall %} -
- {{ api_key(current_service.id, "Service ID", thing="service ID") }} -
- {{ page_footer( secondary_link=url_for('.api_integration', service_id=current_service.id), secondary_link_text='Back to API integration' diff --git a/app/templates/views/api/keys/show.html b/app/templates/views/api/keys/show.html index 3191f8b31..645b0e07c 100644 --- a/app/templates/views/api/keys/show.html +++ b/app/templates/views/api/keys/show.html @@ -18,11 +18,31 @@ once you leave this page.

- {{ api_key(secret, key_name) }} +
- {{ page_footer( - secondary_link=url_for('.api_keys', service_id=current_service.id), - secondary_link_text='Back to API keys' - ) }} + {{ api_key( + '{}-{}-{}'.format(key_name, service_id, secret), + 'API key' + ) }} + + {{ page_footer( + secondary_link=url_for('.api_keys', service_id=current_service.id), + secondary_link_text='Back to API keys' + ) }} + +
+ +

For older API clients

+ +

+ If the client you’re using needs a service ID and an API key, + use these values: +

+ +
+ {{ api_key(service_id, 'Service ID', thing='service ID') }} +
+ + {{ api_key(secret, 'API key') }} {% endblock %} diff --git a/app/utils.py b/app/utils.py index 4c8750f39..66d787ce0 100644 --- a/app/utils.py +++ b/app/utils.py @@ -134,10 +134,10 @@ def generate_previous_next_dict(view, service_id, page, title, url_args): } -def email_safe(string): +def email_safe(string, whitespace='.'): return "".join([ - character.lower() if character.isalnum() or character == "." else "" - for character in re.sub(r"\s+", ".", string.strip()) + character.lower() if character.isalnum() or character == whitespace else "" + for character in re.sub(r"\s+", whitespace, string.strip()) ]) diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py index 84789ac61..0de62599f 100644 --- a/tests/app/main/views/test_api_keys.py +++ b/tests/app/main/views/test_api_keys.py @@ -159,15 +159,23 @@ def test_should_create_api_key_with_type_normal(app_, response = client.post( url_for('main.create_api_key', service_id=service_id), data={ - 'key_name': 'some default key name', + 'key_name': 'Some default key name 1/2', 'key_type': 'normal' } ) assert response.status_code == 200 - assert 'some default key name' in response.get_data(as_text=True) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + keys = page.find_all('span', {'class': 'api-key-key'}) + for index, key in enumerate([ + 'some_default_key_name_12-{}-{}'.format(service_id, fake_uuid), + service_id, + fake_uuid + ]): + assert keys[index].text.strip() == key + post.assert_called_once_with(url='/service/{}/api-key'.format(service_id), data={ - 'name': 'some default key name', + 'name': 'Some default key name 1/2', 'key_type': 'normal', 'created_by': api_user_active.id })