From 9784a9936cedbf12076d82384c2411895fbe2aab Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 19 Jan 2016 09:55:13 +0000 Subject: [PATCH 1/8] Add pages for create/view/revoke API keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copying what they’ve done on GOV.UK Pay, we should let users: - generate as many keys as they want - only see the key at time of creation - give keys a name - revoke any key at any time (this should be a one way operation) And based on discussions with @minglis and @servingUpAces, the keys should be used in conjunction with some kind of service ID, which gets encrypted with the key. In other words the secret itself never gets sent over the wire. This commit adds the UI (but not the underlying API integration) for doing the above. --- app/assets/javascripts/apiKey.js | 23 ++--- .../stylesheets/components/api-key.scss | 5 ++ app/assets/stylesheets/components/table.scss | 11 +++ app/main/forms.py | 4 + app/main/views/api_keys.py | 52 ++++++++++- app/templates/components/api-key.html | 5 +- app/templates/components/table.html | 4 + app/templates/main_nav.html | 3 +- app/templates/views/api-keys.html | 87 ++++++++++--------- app/templates/views/api-keys/create.html | 24 +++++ app/templates/views/api-keys/revoke.html | 37 ++++++++ app/templates/views/api-keys/show.html | 33 +++++++ app/templates/views/documentation.html | 49 +++++++++++ tests/app/main/views/test_api_keys.py | 82 ++++++++++++++++- 14 files changed, 356 insertions(+), 63 deletions(-) create mode 100644 app/templates/views/api-keys/create.html create mode 100644 app/templates/views/api-keys/revoke.html create mode 100644 app/templates/views/api-keys/show.html create mode 100644 app/templates/views/documentation.html diff --git a/app/assets/javascripts/apiKey.js b/app/assets/javascripts/apiKey.js index 30d87278d..215be5672 100644 --- a/app/assets/javascripts/apiKey.js +++ b/app/assets/javascripts/apiKey.js @@ -1,15 +1,11 @@ (function(Modules) { "use strict"; + if (!document.queryCommandSupported('copy')) return; + Modules.ApiKey = function() { const states = { - 'initial': ` - - `, - 'keyVisibleBasic': key => ` - ${key} - `, 'keyVisible': key => ` ${key} @@ -33,23 +29,22 @@ this.start = function(component) { - const $component = $(component).html(states.initial).attr('aria-live', 'polite'), + const $component = $(component), key = $component.data('key'); $component - .on( - 'click', '.api-key-button-show', () => - $component.html( - document.queryCommandSupported('copy') ? - states.keyVisible(key) : states.keyVisibleBasic(key) - ) - ) + .html(states.keyVisible(key)) + .attr('aria-live', 'polite') .on( 'click', '.api-key-button-copy', () => this.copyKey( $('.api-key-key', component)[0], () => $component.html(states.keyCopied) ) + ) + .on( + 'click', '.api-key-button-show', () => + $component.html(states.keyVisible(key)) ); }; diff --git a/app/assets/stylesheets/components/api-key.scss b/app/assets/stylesheets/components/api-key.scss index 22c787355..e17b45ef4 100644 --- a/app/assets/stylesheets/components/api-key.scss +++ b/app/assets/stylesheets/components/api-key.scss @@ -1,5 +1,10 @@ .api-key { + &-name { + @include bold-19; + margin-bottom: 5px; + } + &-key { font-family: monospace; display: block; diff --git a/app/assets/stylesheets/components/table.scss b/app/assets/stylesheets/components/table.scss index ed749b6e3..ce6930370 100644 --- a/app/assets/stylesheets/components/table.scss +++ b/app/assets/stylesheets/components/table.scss @@ -1,3 +1,7 @@ +.table { + margin-bottom: $gutter; +} + .table-heading { text-align: left; } @@ -16,8 +20,15 @@ } &-error { + color: $error-colour; font-weight: bold; + + a:link, + a:visited { + color: $error-colour; + } + } } diff --git a/app/main/forms.py b/app/main/forms.py index 70af82666..6b8a224a2 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -235,3 +235,7 @@ class ChangeMobileNumberForm(Form): class ConfirmMobileNumberForm(Form): sms_code = sms_code() + + +class CreateKeyForm(Form): + key_name = StringField(u'Description of key') diff --git a/app/main/views/api_keys.py b/app/main/views/api_keys.py index 0241346a8..74a5abab4 100644 --- a/app/main/views/api_keys.py +++ b/app/main/views/api_keys.py @@ -1,9 +1,57 @@ -from flask import render_template +from flask import request, render_template, redirect, url_for, flash from flask_login import login_required from app.main import main +from app.main.forms import CreateKeyForm + + +@main.route("/services//documentation") +@login_required +def documentation(service_id): + return render_template('views/documentation.html', service_id=service_id) @main.route("/services//api-keys") @login_required def api_keys(service_id): - return render_template('views/api-keys.html', service_id=service_id) + return render_template( + 'views/api-keys.html', + service_id=service_id, + keys=[ + {'name': 'Test key 1', 'last_used': '12 January 2016, 10:01AM', 'id': 1}, + {'name': 'Test key 2', 'last_used': '12 January 2016, 9:50AM', 'id': 1}, + {'name': 'Test key 3', 'last_used': '12 January 2016, 9:49AM', 'id': 1}, + { + 'name': 'My first key', 'last_used': '25 December 2015, 09:49AM', 'id': 1, + 'revoked': '4 January 2016, 6:00PM' + } + ] + ) + + +@main.route("/services//api-keys/create", methods=['GET', 'POST']) +@login_required +def create_api_key(service_id): + form = CreateKeyForm() + if form.validate_on_submit(): + return redirect(url_for('.show_api_key', service_id=service_id)) + return render_template( + 'views/api-keys/create.html', + service_id=service_id, + key_name=form.key_name + ) + + +@main.route("/services//api-keys/show") +@login_required +def show_api_key(service_id): + return render_template('views/api-keys/show.html', service_id=service_id) + + +@main.route("/services//api-keys/revoke/", methods=['GET', 'POST']) +@login_required +def revoke_api_key(service_id, key_id): + if request.method == 'GET': + return render_template('views/api-keys/revoke.html', service_id=service_id) + elif request.method == 'POST': + flash('‘Test key 1’ was revoked') + return redirect(url_for('.api_keys', service_id=service_id)) diff --git a/app/templates/components/api-key.html b/app/templates/components/api-key.html index 014136bc4..2a2eb2525 100644 --- a/app/templates/components/api-key.html +++ b/app/templates/components/api-key.html @@ -1,4 +1,7 @@ -{% macro api_key(key) %} +{% macro api_key(key, name) %} +

+ {{ name }} +

{{ key }}
diff --git a/app/templates/components/table.html b/app/templates/components/table.html index fb41a6aa7..b5fe9e9d2 100644 --- a/app/templates/components/table.html +++ b/app/templates/components/table.html @@ -55,3 +55,7 @@ {% macro right_aligned_field_heading(text) %} {{ text }} {%- endmacro %} + +{% macro hidden_field_heading(text) %} + {{ text }} +{%- endmacro %} diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index 65cc28c57..d78cdd3ca 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -9,7 +9,8 @@
  • Templates
    • Manage users
      • diff --git a/app/templates/views/api-keys.html b/app/templates/views/api-keys.html index 7cc837971..601deead0 100644 --- a/app/templates/views/api-keys.html +++ b/app/templates/views/api-keys.html @@ -1,6 +1,5 @@ {% extends "withnav_template.html" %} -{% from "components/page-footer.html" import page_footer %} -{% from "components/api-key.html" import api_key %} +{% from "components/table.html" import list_table, field, hidden_field_heading %} {% block page_title %} GOV.UK Notify | API keys and documentation @@ -8,49 +7,55 @@ {% block maincolumn_content %} -
      -
      +

      + API keys +

      -

      - API keys and documentation -

      +

      + To connect to the API you will need to send your service ID, encrypted with + an API key. The API key stays secret. +

      -

      - How to integrate GOV.UK Notify into your service -

      +

      + There are client libraries for several languages which manage this for + you. See + the + developer documentation for more information. +

      -

      - blah blah blah this is where we tell you how the API works -

      +

      + Service ID +

      +

      + {{ service_id }} +

      -

      Repositories

      + {% call(item) list_table( + keys, + empty_message="You haven’t created any API keys yet", + caption="API keys", + caption_visible=False, + field_headings=['Key name', 'Created at', hidden_field_heading('Action')] + ) %} + {% call field() %} + {{ item.name }} + {% endcall %} + {% call field() %} + {{ item.last_used }} + {% endcall %} + {% if item.revoked %} + {% call field(align='right', status='default') %} + Revoked {{ item.revoked }} + {% endcall %} + {% else %} + {% call field(align='right', status='error') %} + Revoke + {% endcall %} + {% endif %} + {% endcall %} - - -

      - GOV.UK Notify API -

      - -

      - GOV.UK Notify Python client -

      - -

      API key for [service name]

      - - {{ api_key('d30512af92e1386d63b90e5973b49a10') }} - -

      API endpoint

      - -

      - https://www.notify.works/api/endpoint -

      - -
      -
      - - {{ page_footer( - back_link=url_for('.service_dashboard', service_id=service_id), - back_link_text='Back to dashboard' - ) }} +

      + Create a new API key +

      {% endblock %} diff --git a/app/templates/views/api-keys/create.html b/app/templates/views/api-keys/create.html new file mode 100644 index 000000000..570a54194 --- /dev/null +++ b/app/templates/views/api-keys/create.html @@ -0,0 +1,24 @@ +{% extends "withnav_template.html" %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/textbox.html" import textbox %} + +{% block page_title %} + GOV.UK Notify | API keys and documentation +{% endblock %} + +{% block maincolumn_content %} + +

      + Add a new API key +

      + +
      + {{ textbox(key_name, hint='eg CRM application') }} + {{ page_footer( + 'Continue', + back_link=url_for('.api_keys', service_id=service_id), + back_link_text='Back to API keys' + ) }} +
      + +{% endblock %} diff --git a/app/templates/views/api-keys/revoke.html b/app/templates/views/api-keys/revoke.html new file mode 100644 index 000000000..3597a6270 --- /dev/null +++ b/app/templates/views/api-keys/revoke.html @@ -0,0 +1,37 @@ +{% extends "withnav_template.html" %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/api-key.html" import api_key %} + +{% block page_title %} + GOV.UK Notify | API keys and documentation +{% endblock %} + +{% block maincolumn_content %} + +
      +
      + +

      + Revoke API key +

      + +

      + ‘Test key 1’ will no longer let you connect to GOV.UK Notify. +

      +

      + You can’t undo this. +

      + +
      +
      + +
      + {{ page_footer( + 'Revoke this API key', + back_link=url_for('.api_keys', service_id=service_id), + back_link_text='Back to API keys', + destructive=True + ) }} +
      + +{% endblock %} diff --git a/app/templates/views/api-keys/show.html b/app/templates/views/api-keys/show.html new file mode 100644 index 000000000..fca59852d --- /dev/null +++ b/app/templates/views/api-keys/show.html @@ -0,0 +1,33 @@ +{% extends "withnav_template.html" %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/api-key.html" import api_key %} + +{% block page_title %} + GOV.UK Notify | API keys and documentation +{% endblock %} + +{% block maincolumn_content %} + +
      +
      + +

      + New API key +

      + +

      + Copy your key to somewhere safe. You won’t be able to see it again + once you leave this page. +

      + + {{ api_key('d30512af92e1386d63b90e5973b49a10', 'CRM application') }} + +
      +
      + + {{ page_footer( + back_link=url_for('.api_keys', service_id=service_id), + back_link_text='Back to API keys' + ) }} + +{% endblock %} diff --git a/app/templates/views/documentation.html b/app/templates/views/documentation.html new file mode 100644 index 000000000..944c84b5c --- /dev/null +++ b/app/templates/views/documentation.html @@ -0,0 +1,49 @@ +{% extends "withnav_template.html" %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/api-key.html" import api_key %} + +{% block page_title %} + GOV.UK Notify | API keys and documentation +{% endblock %} + +{% block maincolumn_content %} + +
      +
      + +

      + Developer documentation +

      + +

      + How to integrate GOV.UK Notify into your service +

      + +

      + blah blah blah this is where we tell you how the API works +

      + +

      Repositories

      + +

      + GOV.UK Notify API +

      + +

      + GOV.UK Notify Python client +

      + +

      API endpoint

      + +

      + https://www.notify.works/api/endpoint +

      + +

      + API keys for your service +

      + +
      +
      + +{% endblock %} diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py index 4b863f889..1e274486d 100644 --- a/tests/app/main/views/test_api_keys.py +++ b/tests/app/main/views/test_api_keys.py @@ -1,13 +1,87 @@ from flask import url_for -def test_should_show_api_keys_and_documentation_page(app_, - db_, - db_session, - active_user): +def test_should_show_documentation_page(app_, + db_, + db_session, + active_user): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(active_user) + response = client.get(url_for('main.documentation', service_id=123)) + + assert response.status_code == 200 + + +def test_should_show_api_keys_page(app_, + db_, + db_session, + active_user): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user) response = client.get(url_for('main.api_keys', service_id=123)) assert response.status_code == 200 + + +def test_should_show_name_api_key_page(app_, + db_, + db_session, + active_user): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(active_user) + response = client.get(url_for('main.create_api_key', service_id=123)) + + assert response.status_code == 200 + + +def test_should_redirect_to_new_api_key(app_, + db_, + db_session, + active_user): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(active_user) + response = client.post(url_for('main.create_api_key', service_id=123)) + + assert response.status_code == 302 + assert response.location == url_for('main.show_api_key', service_id=123, _external=True) + + +def test_should_show_new_api_key(app_, + db_, + db_session, + active_user): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(active_user) + response = client.get(url_for('main.show_api_key', service_id=123)) + + assert response.status_code == 200 + + +def test_should_show_confirm_revoke_api_key(app_, + db_, + db_session, + active_user): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(active_user) + response = client.get(url_for('main.revoke_api_key', service_id=123, key_id=321)) + + assert response.status_code == 200 + + +def test_should_redirect_after_revoking_api_key(app_, + db_, + db_session, + active_user): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(active_user) + response = client.post(url_for('main.revoke_api_key', service_id=123, key_id=321)) + + assert response.status_code == 302 + assert response.location == url_for('.api_keys', service_id=123, _external=True) From 41c775cd686d4710f3ee3e476c579c500d2be56d Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 20 Jan 2016 17:32:55 +0000 Subject: [PATCH 2/8] Created api_key_api_client. Implementation of create, revoke and show api keys for service. These calls work, however we still need to fix the tests. --- app/__init__.py | 3 +++ app/main/views/api_keys.py | 16 ++++++---------- app/notify_client/api_key_api_client.py | 24 ++++++++++++++++++++++++ app/templates/views/api-keys.html | 11 ++++------- app/templates/views/api-keys/show.html | 2 +- tests/conftest.py | 4 ++++ 6 files changed, 42 insertions(+), 18 deletions(-) create mode 100644 app/notify_client/api_key_api_client.py diff --git a/app/__init__.py b/app/__init__.py index 7395965b8..1914a4adb 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -8,6 +8,7 @@ from flask_login import LoginManager from flask_wtf import CsrfProtect from werkzeug.exceptions import abort from app.notify_client.api_client import NotificationsAdminAPIClient +from app.notify_client.api_key_api_client import ApiKeyApiClient from app.notify_client.user_api_client import UserApiClient from app.its_dangerous_session import ItsdangerousSessionInterface import app.proxy_fix @@ -20,6 +21,7 @@ csrf = CsrfProtect() notifications_api_client = NotificationsAdminAPIClient() user_api_client = UserApiClient() +api_key_api_client = ApiKeyApiClient() def create_app(config_name, config_overrides=None): @@ -34,6 +36,7 @@ def create_app(config_name, config_overrides=None): notifications_api_client.init_app(application) user_api_client.init_app(application) + api_key_api_client.init_app(application) login_manager.init_app(application) login_manager.login_view = 'main.sign_in' diff --git a/app/main/views/api_keys.py b/app/main/views/api_keys.py index 74a5abab4..d32332a2b 100644 --- a/app/main/views/api_keys.py +++ b/app/main/views/api_keys.py @@ -2,6 +2,7 @@ from flask import request, render_template, redirect, url_for, flash from flask_login import login_required from app.main import main from app.main.forms import CreateKeyForm +from app import api_key_api_client @main.route("/services//documentation") @@ -16,15 +17,7 @@ def api_keys(service_id): return render_template( 'views/api-keys.html', service_id=service_id, - keys=[ - {'name': 'Test key 1', 'last_used': '12 January 2016, 10:01AM', 'id': 1}, - {'name': 'Test key 2', 'last_used': '12 January 2016, 9:50AM', 'id': 1}, - {'name': 'Test key 3', 'last_used': '12 January 2016, 9:49AM', 'id': 1}, - { - 'name': 'My first key', 'last_used': '25 December 2015, 09:49AM', 'id': 1, - 'revoked': '4 January 2016, 6:00PM' - } - ] + keys=api_key_api_client.get_api_keys(service_id=service_id)['apiKeys'] ) @@ -33,7 +26,9 @@ def api_keys(service_id): def create_api_key(service_id): form = CreateKeyForm() if form.validate_on_submit(): - return redirect(url_for('.show_api_key', service_id=service_id)) + secret = api_key_api_client.create_api_key(service_id=service_id, key_name=form.key_name.data) + return render_template('views/api-keys/show.html', service_id=service_id, secret=secret, + key_name=form.key_name.data) return render_template( 'views/api-keys/create.html', service_id=service_id, @@ -53,5 +48,6 @@ def revoke_api_key(service_id, key_id): if request.method == 'GET': return render_template('views/api-keys/revoke.html', service_id=service_id) elif request.method == 'POST': + api_key_api_client.revoke_api_key(service_id=service_id, key_id=key_id) flash('‘Test key 1’ was revoked') return redirect(url_for('.api_keys', service_id=service_id)) diff --git a/app/notify_client/api_key_api_client.py b/app/notify_client/api_key_api_client.py new file mode 100644 index 000000000..d538cb816 --- /dev/null +++ b/app/notify_client/api_key_api_client.py @@ -0,0 +1,24 @@ +from client.base import BaseAPIClient + + +class ApiKeyApiClient(BaseAPIClient): + def __init__(self, base_url=None, client_id=None, secret=None): + super(self.__class__, self).__init__(base_url=base_url or 'base_url', + client_id=client_id or 'client_id', + secret=secret or 'secret') + + def init_app(self, app): + self.base_url = app.config['API_HOST_NAME'] + self.client_id = app.config['ADMIN_CLIENT_USER_NAME'] + self.secret = app.config['ADMIN_CLIENT_SECRET'] + + def get_api_keys(self, service_id, *params): + return self.get(url='/service/{}/api-keys'.format(service_id)) + + def create_api_key(self, service_id, key_name, *params): + data = {"name": key_name} + key = self.post(url='/service/{}/api-key'.format(service_id), data=data) + return key['data'] + + def revoke_api_key(self, service_id, key_id, *params): + return self.post(url='/service/{0}/api-key/revoke/{1}'.format(service_id, key_id), data=None) \ No newline at end of file diff --git a/app/templates/views/api-keys.html b/app/templates/views/api-keys.html index 601deead0..dbd48cab0 100644 --- a/app/templates/views/api-keys.html +++ b/app/templates/views/api-keys.html @@ -35,21 +35,18 @@ empty_message="You haven’t created any API keys yet", caption="API keys", caption_visible=False, - field_headings=['Key name', 'Created at', hidden_field_heading('Action')] + field_headings=['Key name', hidden_field_heading('Action')] ) %} {% call field() %} {{ item.name }} {% endcall %} - {% call field() %} - {{ item.last_used }} - {% endcall %} - {% if item.revoked %} + {% if item.expiry_date %} {% call field(align='right', status='default') %} - Revoked {{ item.revoked }} + Revoked {{ item.expiry_date }} {% endcall %} {% else %} {% call field(align='right', status='error') %} - Revoke + Revoke {% endcall %} {% endif %} {% endcall %} diff --git a/app/templates/views/api-keys/show.html b/app/templates/views/api-keys/show.html index fca59852d..a19a038b2 100644 --- a/app/templates/views/api-keys/show.html +++ b/app/templates/views/api-keys/show.html @@ -20,7 +20,7 @@ once you leave this page.

      - {{ api_key('d30512af92e1386d63b90e5973b49a10', 'CRM application') }} + {{ api_key(secret, key_name) }} diff --git a/tests/conftest.py b/tests/conftest.py index ab4890253..7bbf805d8 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -210,3 +210,7 @@ def mock_register_user(mocker, user_data): mock_class = mocker.patch('app.user_api_client.register_user') mock_class.return_value = user return mock_class + + +def mock_create_api_key(mocker, key_name): + mock_class = mocker.patch('app.api') From 90fca933084f2458c2583f34d094f17ee2c62e6a Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 21 Jan 2016 12:28:05 +0000 Subject: [PATCH 3/8] Implementation of api key pages. Revoke page will show the correct key name Show api keys shows a well formatted expiry date Fix tests for api key endpoints. --- app/__init__.py | 8 +++ app/main/views/api_keys.py | 15 +++--- app/notify_client/api_key_api_client.py | 9 ++-- app/templates/views/api-keys.html | 2 +- app/templates/views/api-keys/revoke.html | 2 +- tests/__init__.py | 6 +++ tests/app/main/views/test_api_keys.py | 68 +++++++++++++++--------- tests/conftest.py | 59 ++++++++++++++++++-- 8 files changed, 127 insertions(+), 42 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 1914a4adb..80da769a3 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -1,6 +1,7 @@ import os import re +import dateutil from flask import Flask, session, Markup, escape, render_template from flask._compat import string_types from flask.ext.sqlalchemy import SQLAlchemy @@ -54,6 +55,7 @@ def create_app(config_name, config_overrides=None): application.add_template_filter(placeholders) application.add_template_filter(replace_placeholders) application.add_template_filter(nl2br) + application.add_template_filter(format_datetime) application.after_request(useful_headers_after_request) register_errorhandlers(application) @@ -134,6 +136,12 @@ def replace_placeholders(template, values): )) +def format_datetime(date): + date = dateutil.parser.parse(date) + native = date.replace(tzinfo=None) + return native.strftime('%A %d %B %Y at %H:%M') + + # https://www.owasp.org/index.php/List_of_useful_HTTP_headers def useful_headers_after_request(response): response.headers.add('X-Frame-Options', 'deny') diff --git a/app/main/views/api_keys.py b/app/main/views/api_keys.py index d32332a2b..f3790d174 100644 --- a/app/main/views/api_keys.py +++ b/app/main/views/api_keys.py @@ -36,18 +36,17 @@ def create_api_key(service_id): ) -@main.route("/services//api-keys/show") -@login_required -def show_api_key(service_id): - return render_template('views/api-keys/show.html', service_id=service_id) - - @main.route("/services//api-keys/revoke/", methods=['GET', 'POST']) @login_required 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'] if request.method == 'GET': - return render_template('views/api-keys/revoke.html', service_id=service_id) + return render_template( + 'views/api-keys/revoke.html', + service_id=service_id, + key_name=key_name + ) elif request.method == 'POST': api_key_api_client.revoke_api_key(service_id=service_id, key_id=key_id) - flash('‘Test key 1’ was revoked') + flash('‘{}’ was revoked'.format(key_name)) return redirect(url_for('.api_keys', service_id=service_id)) diff --git a/app/notify_client/api_key_api_client.py b/app/notify_client/api_key_api_client.py index d538cb816..660f839c8 100644 --- a/app/notify_client/api_key_api_client.py +++ b/app/notify_client/api_key_api_client.py @@ -12,8 +12,11 @@ class ApiKeyApiClient(BaseAPIClient): self.client_id = app.config['ADMIN_CLIENT_USER_NAME'] self.secret = app.config['ADMIN_CLIENT_SECRET'] - def get_api_keys(self, service_id, *params): - return self.get(url='/service/{}/api-keys'.format(service_id)) + def get_api_keys(self, service_id, key_id=None, *params): + 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 create_api_key(self, service_id, key_name, *params): data = {"name": key_name} @@ -21,4 +24,4 @@ class ApiKeyApiClient(BaseAPIClient): return key['data'] def revoke_api_key(self, service_id, key_id, *params): - return self.post(url='/service/{0}/api-key/revoke/{1}'.format(service_id, key_id), data=None) \ No newline at end of file + return self.post(url='/service/{0}/api-key/revoke/{1}'.format(service_id, key_id), data=None) diff --git a/app/templates/views/api-keys.html b/app/templates/views/api-keys.html index dbd48cab0..1886cd8ac 100644 --- a/app/templates/views/api-keys.html +++ b/app/templates/views/api-keys.html @@ -42,7 +42,7 @@ {% endcall %} {% if item.expiry_date %} {% call field(align='right', status='default') %} - Revoked {{ item.expiry_date }} + Revoked {{ item.expiry_date|format_datetime }} {% endcall %} {% else %} {% call field(align='right', status='error') %} diff --git a/app/templates/views/api-keys/revoke.html b/app/templates/views/api-keys/revoke.html index 3597a6270..abf066c65 100644 --- a/app/templates/views/api-keys/revoke.html +++ b/app/templates/views/api-keys/revoke.html @@ -16,7 +16,7 @@

      - ‘Test key 1’ will no longer let you connect to GOV.UK Notify. + ‘{{ key_name }}’ will no longer let you connect to GOV.UK Notify.

      You can’t undo this. diff --git a/tests/__init__.py b/tests/__init__.py index fc5f15fb3..c0806dcbb 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -39,6 +39,12 @@ def template_json(id_, name, type_, content, service_id): } +def api_key_json(id_, name, expiry_date=None): + return {'id': id_, + 'name': name, + 'expiry_date': expiry_date + } + TEST_USER_EMAIL = 'test@user.gov.uk' diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py index 1e274486d..94ef8fe41 100644 --- a/tests/app/main/views/test_api_keys.py +++ b/tests/app/main/views/test_api_keys.py @@ -1,3 +1,4 @@ +from datetime import date from flask import url_for @@ -13,16 +14,37 @@ def test_should_show_documentation_page(app_, assert response.status_code == 200 -def test_should_show_api_keys_page(app_, - db_, - db_session, - active_user): +def test_should_show_empty_api_keys_page(app_, + db_, + db_session, + active_user, + mock_get_no_api_keys): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user) response = client.get(url_for('main.api_keys', service_id=123)) assert response.status_code == 200 + assert 'You haven’t created any API keys yet' in response.get_data(as_text=True) + assert 'Create a new API key' in response.get_data(as_text=True) + mock_get_no_api_keys.assert_called_once_with(service_id=123) + + +def test_should_show_api_keys_page(app_, + db_, + db_session, + active_user, + mock_get_api_keys): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(active_user) + response = client.get(url_for('main.api_keys', service_id=123)) + + assert response.status_code == 200 + assert 'some key name' in response.get_data(as_text=True) + assert 'another key name' in response.get_data(as_text=True) + assert 'Revoked Thursday 01 January 1970 at 00:00' in response.get_data(as_text=True) + mock_get_api_keys.assert_called_once_with(service_id=123) def test_should_show_name_api_key_page(app_, @@ -37,47 +59,43 @@ def test_should_show_name_api_key_page(app_, assert response.status_code == 200 -def test_should_redirect_to_new_api_key(app_, - db_, - db_session, - active_user): +def test_should_render_show_api_key(app_, + db_, + db_session, + active_user, + mock_create_api_key): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user) - response = client.post(url_for('main.create_api_key', service_id=123)) - - assert response.status_code == 302 - assert response.location == url_for('main.show_api_key', service_id=123, _external=True) - - -def test_should_show_new_api_key(app_, - db_, - db_session, - active_user): - with app_.test_request_context(): - with app_.test_client() as client: - client.login(active_user) - response = client.get(url_for('main.show_api_key', service_id=123)) + response = client.post(url_for('main.create_api_key', service_id=123), + data={'key_name': 'some default key name'}) assert response.status_code == 200 + assert 'some default key name' in response.get_data(as_text=True) + mock_create_api_key.assert_called_once_with(service_id=123, key_name='some default key name') def test_should_show_confirm_revoke_api_key(app_, db_, db_session, - active_user): + active_user, + mock_get_api_keys): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user) response = client.get(url_for('main.revoke_api_key', service_id=123, key_id=321)) assert response.status_code == 200 + assert 'some key name' in response.get_data(as_text=True) + mock_get_api_keys.assert_called_once_with(service_id=123, key_id=321) def test_should_redirect_after_revoking_api_key(app_, db_, db_session, - active_user): + active_user, + mock_revoke_api_key, + mock_get_api_keys): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user) @@ -85,3 +103,5 @@ def test_should_redirect_after_revoking_api_key(app_, assert response.status_code == 302 assert response.location == url_for('.api_keys', service_id=123, _external=True) + mock_revoke_api_key.assert_called_once_with(service_id=123, key_id=321) + mock_get_api_keys.assert_called_once_with(service_id=123, key_id=321) diff --git a/tests/conftest.py b/tests/conftest.py index 7bbf805d8..ab89761f1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,14 +1,17 @@ import os +from datetime import date + import pytest from alembic.command import upgrade from alembic.config import Config from flask.ext.migrate import Migrate, MigrateCommand from flask.ext.script import Manager from sqlalchemy.schema import MetaData -from . import ( - create_test_user, create_another_test_user, service_json, TestClient, - get_test_user, template_json) + from app import create_app, db +from . import ( + create_test_user, service_json, TestClient, + get_test_user, template_json, api_key_json) @pytest.fixture(scope='session') @@ -89,6 +92,7 @@ def mock_get_service(mocker, active_user): service_id, "Test Service", [active_user.id], limit=1000, active=False, restricted=True) return {'data': service, 'token': 1} + return mocker.patch('app.notifications_api_client.get_service', side_effect=_create) @@ -99,6 +103,7 @@ def mock_create_service(mocker): 101, service_name, [user_id], limit=limit, active=active, restricted=restricted) return {'data': service} + mock_class = mocker.patch( 'app.notifications_api_client.create_service', side_effect=_create) return mock_class @@ -116,6 +121,7 @@ def mock_update_service(mocker): service_id, service_name, users, limit=limit, active=active, restricted=restricted) return {'data': service} + mock_class = mocker.patch( 'app.notifications_api_client.update_service', side_effect=_update) return mock_class @@ -129,6 +135,7 @@ def mock_get_services(mocker, active_user): service_two = service_json( 2, "service_two", [active_user.id], 1000, True, False) return {'data': [service_one, service_two]} + mock_class = mocker.patch( 'app.notifications_api_client.get_services', side_effect=_create) return mock_class @@ -138,6 +145,7 @@ def mock_get_services(mocker, active_user): def mock_delete_service(mocker, mock_get_service): def _delete(service_id): return mock_get_service.side_effect(service_id) + mock_class = mocker.patch( 'app.notifications_api_client.delete_service', side_effect=_delete) return mock_class @@ -149,6 +157,7 @@ def mock_get_service_template(mocker): template = template_json( template_id, "Template Name", "sms", "template content", service_id) return {'data': template} + return mocker.patch( 'app.notifications_api_client.get_service_template', side_effect=_create) @@ -160,6 +169,7 @@ def mock_create_service_template(mocker): template = template_json( 101, name, type_, content, service) return {'data': template} + mock_class = mocker.patch( 'app.notifications_api_client.create_service_template', side_effect=_create) @@ -172,6 +182,7 @@ def mock_update_service_template(mocker): template = template_json( id_, name, type_, content, service) return {'data': template} + mock_class = mocker.patch( 'app.notifications_api_client.update_service_template', side_effect=_update) @@ -186,6 +197,7 @@ def mock_get_service_templates(mocker): template_two = template_json( 2, "template_two", "sms", "template two content", service_id) return {'data': [template_one, template_two]} + mock_class = mocker.patch( 'app.notifications_api_client.get_service_templates', side_effect=_create) @@ -199,6 +211,7 @@ def mock_delete_service_template(mocker): template_id, "Template to delete", "sms", "content to be deleted", service_id) return {'data': template} + return mocker.patch( 'app.notifications_api_client.delete_service_template', side_effect=_delete) @@ -212,5 +225,41 @@ def mock_register_user(mocker, user_data): return mock_class -def mock_create_api_key(mocker, key_name): - mock_class = mocker.patch('app.api') +@pytest.fixture(scope='function') +def mock_create_api_key(mocker): + def _create(service_id, key_name): + import uuid + return {'data': str(uuid.uuid4())} + + mock_class = mocker.patch('app.api_key_api_client.create_api_key', side_effect=_create) + return mock_class + + +@pytest.fixture(scope='function') +def mock_revoke_api_key(mocker): + def _revoke(service_id, key_id): + return {} + + mock_class = mocker.patch('app.api_key_api_client.revoke_api_key', side_effect=_revoke) + return mock_class + + +@pytest.fixture(scope='function') +def mock_get_api_keys(mocker): + def _get_keys(service_id, key_id=None): + keys = {'apiKeys': [api_key_json(1, 'some key name'), + api_key_json(2, 'another key name', expiry_date=str(date.fromtimestamp(0)))]} + return keys + + mock_class = mocker.patch('app.api_key_api_client.get_api_keys', side_effect=_get_keys) + return mock_class + + +@pytest.fixture(scope='function') +def mock_get_no_api_keys(mocker): + def _get_keys(service_id): + keys = {'apiKeys': []} + return keys + + mock_class = mocker.patch('app.api_key_api_client.get_api_keys', side_effect=_get_keys) + return mock_class From 2b24909ca677bfe230ff47bea8d2f0ed0131502c Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 21 Jan 2016 12:43:11 +0000 Subject: [PATCH 4/8] Fix tests for to use mock user for login --- tests/app/main/views/test_api_keys.py | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py index 94ef8fe41..fbd2054ea 100644 --- a/tests/app/main/views/test_api_keys.py +++ b/tests/app/main/views/test_api_keys.py @@ -5,7 +5,10 @@ from flask import url_for def test_should_show_documentation_page(app_, db_, db_session, - active_user): + active_user, + mock_get_service, + mock_get_services, + mock_user_loader): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user) @@ -18,6 +21,9 @@ def test_should_show_empty_api_keys_page(app_, db_, db_session, active_user, + mock_get_service, + mock_get_services, + mock_user_loader, mock_get_no_api_keys): with app_.test_request_context(): with app_.test_client() as client: @@ -34,6 +40,9 @@ def test_should_show_api_keys_page(app_, db_, db_session, active_user, + mock_get_service, + mock_get_services, + mock_user_loader, mock_get_api_keys): with app_.test_request_context(): with app_.test_client() as client: @@ -50,7 +59,10 @@ def test_should_show_api_keys_page(app_, def test_should_show_name_api_key_page(app_, db_, db_session, - active_user): + active_user, + mock_get_service, + mock_get_services, + mock_user_loader): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user) @@ -63,6 +75,9 @@ def test_should_render_show_api_key(app_, db_, db_session, active_user, + mock_get_service, + mock_get_services, + mock_user_loader, mock_create_api_key): with app_.test_request_context(): with app_.test_client() as client: @@ -79,6 +94,9 @@ def test_should_show_confirm_revoke_api_key(app_, db_, db_session, active_user, + mock_get_service, + mock_get_services, + mock_user_loader, mock_get_api_keys): with app_.test_request_context(): with app_.test_client() as client: @@ -94,6 +112,9 @@ def test_should_redirect_after_revoking_api_key(app_, db_, db_session, active_user, + mock_get_service, + mock_get_services, + mock_user_loader, mock_revoke_api_key, mock_get_api_keys): with app_.test_request_context(): From e7713a8b7f646e3cc0cafcfc791da72988d1c0ed Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 21 Jan 2016 14:15:36 +0000 Subject: [PATCH 5/8] Added a validator so that the key name is unique per service. --- app/main/forms.py | 12 +++++++++++- app/main/views/api_keys.py | 5 ++++- tests/app/main/test_create_api_key_form.py | 16 ++++++++++++++++ tests/app/main/views/test_api_keys.py | 6 ++++-- 4 files changed, 35 insertions(+), 4 deletions(-) create mode 100644 tests/app/main/test_create_api_key_form.py diff --git a/app/main/forms.py b/app/main/forms.py index 6b8a224a2..eb82e9be6 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -238,4 +238,14 @@ class ConfirmMobileNumberForm(Form): class CreateKeyForm(Form): - key_name = StringField(u'Description of key') + def __init__(self, existing_key_names=[], *args, **kwargs): + self.existing_key_names = existing_key_names + super(CreateKeyForm, self).__init__(*args, **kwargs) + + key_name = StringField(u'Description of key', validators=[ + DataRequired(message='You need to give the key a name') + ]) + + def validate_key_name(self, key_name): + if key_name.data in self.existing_key_names: + raise ValidationError('A key with this name already exists') diff --git a/app/main/views/api_keys.py b/app/main/views/api_keys.py index f3790d174..e2d77bf31 100644 --- a/app/main/views/api_keys.py +++ b/app/main/views/api_keys.py @@ -24,7 +24,10 @@ def api_keys(service_id): @main.route("/services//api-keys/create", methods=['GET', 'POST']) @login_required def create_api_key(service_id): - form = CreateKeyForm() + key_names = [ + key['name'] for key in api_key_api_client.get_api_keys(service_id=service_id)['apiKeys'] + ] + form = CreateKeyForm(key_names) if form.validate_on_submit(): secret = api_key_api_client.create_api_key(service_id=service_id, key_name=form.key_name.data) return render_template('views/api-keys/show.html', service_id=service_id, secret=secret, diff --git a/tests/app/main/test_create_api_key_form.py b/tests/app/main/test_create_api_key_form.py new file mode 100644 index 000000000..54608da6f --- /dev/null +++ b/tests/app/main/test_create_api_key_form.py @@ -0,0 +1,16 @@ +from werkzeug.datastructures import MultiDict + +from app.main.forms import CreateKeyForm + + +def test_return_validation_error_when_key_name_exists(app_, + db_, + db_session): + def _get_names(): + return ['some key', 'another key'] + + with app_.test_request_context(): + form = CreateKeyForm(_get_names(), + formdata=MultiDict([('key_name', 'some key')])) + form.validate() + assert {'key_name': ['A key with this name already exists']} == form.errors diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py index fbd2054ea..e1b114398 100644 --- a/tests/app/main/views/test_api_keys.py +++ b/tests/app/main/views/test_api_keys.py @@ -62,7 +62,8 @@ def test_should_show_name_api_key_page(app_, active_user, mock_get_service, mock_get_services, - mock_user_loader): + mock_user_loader, + mock_get_api_keys): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user) @@ -78,7 +79,8 @@ def test_should_render_show_api_key(app_, mock_get_service, mock_get_services, mock_user_loader, - mock_create_api_key): + mock_create_api_key, + mock_get_api_keys): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user) From a66dd52ca8dbe7744482b38ee7dfc5f1e97e1da9 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 21 Jan 2016 15:43:31 +0000 Subject: [PATCH 6/8] Better words --- 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 1886cd8ac..6a65eae71 100644 --- a/app/templates/views/api-keys.html +++ b/app/templates/views/api-keys.html @@ -17,8 +17,7 @@

      - There are client libraries for several languages which manage this for - you. See + There are client libraries available which can do this for you. See the developer documentation for more information.

      From 81a5164fa543f042bac85b0ac404736f8adf05b0 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 21 Jan 2016 15:56:08 +0000 Subject: [PATCH 7/8] Fix test to use mock_api_user --- tests/app/main/views/test_api_keys.py | 43 +++++++++++---------------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py index 60016cebc..71eed1353 100644 --- a/tests/app/main/views/test_api_keys.py +++ b/tests/app/main/views/test_api_keys.py @@ -11,7 +11,6 @@ def test_should_show_api_keys_and_documentation_page(app_, with app_.test_request_context(): with app_.test_client() as client: client.login(mock_api_user) - client.login(active_user) response = client.get(url_for('main.documentation', service_id=123)) assert response.status_code == 200 @@ -20,14 +19,13 @@ def test_should_show_api_keys_and_documentation_page(app_, def test_should_show_empty_api_keys_page(app_, db_, db_session, - active_user, - mock_get_service, - mock_get_services, + mock_api_user, mock_user_loader, + mock_user_dao_get_by_email, mock_get_no_api_keys): with app_.test_request_context(): with app_.test_client() as client: - client.login(active_user) + client.login(mock_api_user) response = client.get(url_for('main.api_keys', service_id=123)) assert response.status_code == 200 @@ -39,14 +37,13 @@ def test_should_show_empty_api_keys_page(app_, def test_should_show_api_keys_page(app_, db_, db_session, - active_user, - mock_get_service, - mock_get_services, + mock_api_user, mock_user_loader, + mock_user_dao_get_by_email, mock_get_api_keys): with app_.test_request_context(): with app_.test_client() as client: - client.login(active_user) + client.login(mock_api_user) response = client.get(url_for('main.api_keys', service_id=123)) assert response.status_code == 200 @@ -59,14 +56,13 @@ def test_should_show_api_keys_page(app_, def test_should_show_name_api_key_page(app_, db_, db_session, - active_user, - mock_get_service, - mock_get_services, + mock_api_user, mock_user_loader, + mock_user_dao_get_by_email, mock_get_api_keys): with app_.test_request_context(): with app_.test_client() as client: - client.login(active_user) + client.login(mock_api_user) response = client.get(url_for('main.create_api_key', service_id=123)) assert response.status_code == 200 @@ -75,15 +71,14 @@ def test_should_show_name_api_key_page(app_, def test_should_render_show_api_key(app_, db_, db_session, - active_user, - mock_get_service, - mock_get_services, + mock_api_user, mock_user_loader, + mock_user_dao_get_by_email, mock_create_api_key, mock_get_api_keys): with app_.test_request_context(): with app_.test_client() as client: - client.login(active_user) + client.login(mock_api_user) response = client.post(url_for('main.create_api_key', service_id=123), data={'key_name': 'some default key name'}) @@ -95,14 +90,13 @@ def test_should_render_show_api_key(app_, def test_should_show_confirm_revoke_api_key(app_, db_, db_session, - active_user, - mock_get_service, - mock_get_services, + mock_api_user, mock_user_loader, + mock_user_dao_get_by_email, mock_get_api_keys): with app_.test_request_context(): with app_.test_client() as client: - client.login(active_user) + client.login(mock_api_user) response = client.get(url_for('main.revoke_api_key', service_id=123, key_id=321)) assert response.status_code == 200 @@ -113,15 +107,14 @@ def test_should_show_confirm_revoke_api_key(app_, def test_should_redirect_after_revoking_api_key(app_, db_, db_session, - active_user, - mock_get_service, - mock_get_services, + mock_api_user, mock_user_loader, + mock_user_dao_get_by_email, mock_revoke_api_key, mock_get_api_keys): with app_.test_request_context(): with app_.test_client() as client: - client.login(active_user) + client.login(mock_api_user) response = client.post(url_for('main.revoke_api_key', service_id=123, key_id=321)) assert response.status_code == 302 From 61893c5c7e89c8c268fa4c249486504d8588cf17 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 21 Jan 2016 16:52:01 +0000 Subject: [PATCH 8/8] Validation on key name is case insenstive --- app/main/forms.py | 4 ++-- tests/app/main/test_create_api_key_form.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index eb82e9be6..fab516607 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -239,7 +239,7 @@ class ConfirmMobileNumberForm(Form): class CreateKeyForm(Form): def __init__(self, existing_key_names=[], *args, **kwargs): - self.existing_key_names = existing_key_names + self.existing_key_names = [x.lower() for x in existing_key_names] super(CreateKeyForm, self).__init__(*args, **kwargs) key_name = StringField(u'Description of key', validators=[ @@ -247,5 +247,5 @@ class CreateKeyForm(Form): ]) def validate_key_name(self, key_name): - if key_name.data in self.existing_key_names: + if key_name.data.lower() in self.existing_key_names: raise ValidationError('A key with this name already exists') diff --git a/tests/app/main/test_create_api_key_form.py b/tests/app/main/test_create_api_key_form.py index 54608da6f..c19114b58 100644 --- a/tests/app/main/test_create_api_key_form.py +++ b/tests/app/main/test_create_api_key_form.py @@ -11,6 +11,6 @@ def test_return_validation_error_when_key_name_exists(app_, with app_.test_request_context(): form = CreateKeyForm(_get_names(), - formdata=MultiDict([('key_name', 'some key')])) + formdata=MultiDict([('key_name', 'Some key')])) form.validate() assert {'key_name': ['A key with this name already exists']} == form.errors