diff --git a/app/main/forms.py b/app/main/forms.py index ae85a7740..9d05ddfbd 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -2551,3 +2551,13 @@ class BroadcastAreaFormWithSelectAll(BroadcastAreaForm): if self.select_all.data: return [self.select_all.area_slug] return self.areas.data + + +class ChangeSecurityKeyNameForm(StripWhitespaceForm): + security_key_name = GovukTextInputField( + 'Name of key', + validators=[ + DataRequired(message='Cannot be empty'), + MustContainAlphanumericCharacters(), + Length(max=255, message='Name of key must be 255 characters or fewer') + ]) diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index f75624df6..f6c701874 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -3,12 +3,15 @@ import json from flask import ( abort, current_app, + flash, redirect, render_template, + request, session, url_for, ) from flask_login import current_user +from notifications_python_client.errors import HTTPError from notifications_utils.url_safe_token import check_token from app import user_api_client @@ -18,6 +21,7 @@ from app.main.forms import ( ChangeMobileNumberForm, ChangeNameForm, ChangePasswordForm, + ChangeSecurityKeyNameForm, ConfirmPasswordForm, ServiceOnOffSettingForm, TwoFactorForm, @@ -237,3 +241,66 @@ def user_profile_security_keys(): return render_template( 'views/user-profile/security-keys.html', ) + + +def get_key_from_list_of_keys(key_id, list_of_keys): + return next((key for key in list_of_keys if key.id == key_id), None) + + +@main.route( + "/user-profile/security-keys//manage", + methods=['GET', 'POST'], + endpoint="user_profile_manage_security_key" +) +@main.route( + "/user-profile/security-keys//delete", + methods=['GET'], + endpoint="user_profile_confirm_delete_security_key" +) +@user_is_platform_admin +def user_profile_manage_security_key(key_id): + security_keys = current_user.webauthn_credentials + security_key = get_key_from_list_of_keys(key_id, security_keys) + + if not security_key: + abort(404) + + form = ChangeSecurityKeyNameForm(security_key_name=security_key.name) + + if form.validate_on_submit(): + if form.security_key_name.data != security_key.name: + user_api_client.update_webauthn_credential_name_for_user( + user_id=current_user.id, + credential_id=key_id, + new_name_for_credential=form.security_key_name.data + ) + return redirect(url_for('.user_profile_security_keys')) + + if (request.endpoint == "main.user_profile_confirm_delete_security_key"): + flash("Are you sure you want to delete this security key?", 'delete') + + return render_template( + 'views/user-profile/manage-security-key.html', + security_key=security_key, + form=form + ) + + +@main.route("/user-profile/security-keys//delete", methods=['POST']) +@user_is_platform_admin +def user_profile_delete_security_key(key_id): + + try: + user_api_client.delete_webauthn_credential_for_user( + user_id=current_user.id, + credential_id=key_id + ) + except HTTPError as e: + message = "Cannot delete last remaining webauthn credential for user" + if e.message == message: + flash("You cannot delete your last security key.") + return redirect(url_for('.user_profile_manage_security_key', key_id=key_id)) + else: + raise e + + return redirect(url_for('.user_profile_security_keys')) diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index df9a2ba47..915b87a2f 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -201,5 +201,15 @@ class UserApiClient(NotifyAdminAPIClient): return self.post(endpoint, data=credential.serialize()) + def update_webauthn_credential_name_for_user(self, *, user_id, credential_id, new_name_for_credential): + endpoint = f'/user/{user_id}/webauthn/{credential_id}' + + return self.post(endpoint, data={"name": new_name_for_credential}) + + def delete_webauthn_credential_for_user(self, *, user_id, credential_id): + endpoint = f'/user/{user_id}/webauthn/{credential_id}' + + return self.delete(endpoint) + user_api_client = UserApiClient() diff --git a/app/templates/views/user-profile/manage-security-key.html b/app/templates/views/user-profile/manage-security-key.html new file mode 100644 index 000000000..7e6c53068 --- /dev/null +++ b/app/templates/views/user-profile/manage-security-key.html @@ -0,0 +1,33 @@ +{% extends "withoutnav_template.html" %} +{% from "components/page-header.html" import page_header %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/form.html" import form_wrapper %} + + +{% set page_title = 'Manage ' + '‘' + security_key.name + '’' %} + +{% block per_page_title %} + {{ page_title }} +{% endblock %} + +{% block maincolumn_content %} + {{ page_header( + page_title, + back_link=url_for('.user_profile_security_keys') + ) }} + +
+
+ {% call form_wrapper(autocomplete=True) %} + {{ form.security_key_name }} + {{ page_footer( + 'Save', + delete_link=url_for( + '.user_profile_confirm_delete_security_key', key_id=security_key.id), + delete_link_text='Delete' + ) }} + {% endcall %} +
+
+ +{% endblock %} diff --git a/app/templates/views/user-profile/security-keys.html b/app/templates/views/user-profile/security-keys.html index c510ff992..c1df142a8 100644 --- a/app/templates/views/user-profile/security-keys.html +++ b/app/templates/views/user-profile/security-keys.html @@ -2,7 +2,7 @@ {% from "components/page-header.html" import page_header %} {% from "components/button/macro.njk" import govukButton %} {% from "components/back-link/macro.njk" import govukBackLink %} -{% from "components/table.html" import mapping_table, row, field, row_heading %} +{% from "components/table.html" import edit_field, mapping_table, row, field, row_heading %} {% from "components/webauthn-api-check.html" import webauthn_api_check %} {% from "vendor/govuk-frontend/components/error-message/macro.njk" import govukErrorMessage %} @@ -26,22 +26,24 @@
{% if credentials %} - - {% call mapping_table( - caption=page_title, - field_headings=['Security key'], - field_headings_visible=False, - caption_visible=False, - ) %} - {% for credential in credentials %} - {% call row() %} - {% call field() %} -
{{ credential.name }}
-
Registered {{ credential.created_at|format_delta }}
+
+ {% call mapping_table( + caption=page_title, + field_headings=['Security key details', 'Action'], + field_headings_visible=False, + caption_visible=False, + ) %} + {% for credential in credentials %} + {% call row() %} + {% call field() %} +
{{ credential.name }}
+
Registered {{ credential.created_at|format_delta }}
+ {% endcall %} + {{ edit_field('Manage', url_for('.user_profile_manage_security_key', key_id=credential.id)) }} {% endcall %} - {% endcall %} - {% endfor %} - {% endcall %} + {% endfor %} + {% endcall %} +
{% else %} diff --git a/tests/app/main/views/test_user_profile.py b/tests/app/main/views/test_user_profile.py index fcf6eb9fc..277a70ef9 100644 --- a/tests/app/main/views/test_user_profile.py +++ b/tests/app/main/views/test_user_profile.py @@ -3,9 +3,16 @@ import uuid import pytest from flask import url_for +from notifications_python_client.errors import HTTPError from notifications_utils.url_safe_token import generate_token -from tests.conftest import create_api_user_active, url_for_endpoint_with_token +from app.main.views.user_profile import get_key_from_list_of_keys +from app.models.user import WebAuthnCredential +from tests.conftest import ( + create_api_user_active, + normalize_spaces, + url_for_endpoint_with_token, +) def test_should_show_overview_page( @@ -366,6 +373,246 @@ def test_should_show_security_keys_page( credential_row = page.select('tr')[-1] assert 'Test credential' in credential_row.text + assert "Manage" in credential_row.find('a').text + assert credential_row.find('a')["href"] == url_for( + '.user_profile_manage_security_key', + key_id=webauthn_credential['id'] + ) register_button = page.select_one("[data-module='register-security-key']") assert register_button.text.strip() == 'Register a key' + + +def test_get_key_from_list_of_keys(webauthn_credential, webauthn_credential_2): + list_of_keys = [WebAuthnCredential(json) for json in [webauthn_credential, webauthn_credential_2]] + assert get_key_from_list_of_keys(webauthn_credential["id"], list_of_keys) == WebAuthnCredential(webauthn_credential) + + +def test_should_show_manage_security_key_page( + mocker, + client_request, + platform_admin_user, + webauthn_credential, +): + client_request.login(platform_admin_user) + + mocker.patch( + 'app.user_api_client.get_webauthn_credentials_for_user', + return_value=[webauthn_credential], + ) + + page = client_request.get('.user_profile_manage_security_key', key_id=webauthn_credential['id']) + assert page.select_one('h1').text.strip() == f'Manage ‘{webauthn_credential["name"]}’' + + assert page.select_one('.govuk-back-link').text.strip() == 'Back' + assert page.select_one('.govuk-back-link')['href'] == url_for('.user_profile_security_keys') + + assert page.select_one('#security_key_name')["value"] == webauthn_credential["name"] + + +def test_manage_security_key_page_404s_when_key_not_found( + mocker, + client_request, + platform_admin_user, + webauthn_credential, + webauthn_credential_2 +): + client_request.login(platform_admin_user) + + mocker.patch( + 'app.user_api_client.get_webauthn_credentials_for_user', + return_value=[webauthn_credential_2], + ) + client_request.get( + '.user_profile_manage_security_key', + key_id=webauthn_credential['id'], + _expected_status=404, + ) + + +@pytest.mark.parametrize('endpoint,method', [ + (".user_profile_manage_security_key", "get"), + (".user_profile_manage_security_key", "post"), + (".user_profile_confirm_delete_security_key", "get"), + (".user_profile_confirm_delete_security_key", "post"), + (".user_profile_delete_security_key", "post"), +]) +def test_non_platform_admin_user_cant_manage_security_keys( + client_request, webauthn_credential, endpoint, method +): + if method == "get": + client_request.get( + endpoint, + key_id=webauthn_credential['id'], + _expected_status=403, + ) + + else: + client_request.post( + endpoint, + key_id=webauthn_credential['id'], + _expected_status=403, + ) + + +def test_should_redirect_after_change_of_security_key_name( + client_request, + platform_admin_user, + webauthn_credential, + mocker +): + client_request.login(platform_admin_user) + + mocker.patch( + 'app.user_api_client.get_webauthn_credentials_for_user', + return_value=[webauthn_credential], + ) + + mock_update = mocker.patch('app.user_api_client.update_webauthn_credential_name_for_user') + + client_request.post( + 'main.user_profile_manage_security_key', + key_id=webauthn_credential['id'], + _data={'security_key_name': "new name"}, + _expected_status=302, + _expected_redirect=url_for( + 'main.user_profile_security_keys', + _external=True, + ) + ) + + mock_update.assert_called_once_with( + credential_id=webauthn_credential['id'], + new_name_for_credential="new name", + user_id=platform_admin_user["id"] + ) + + +def test_user_profile_manage_security_key_should_not_call_api_if_key_name_stays_the_same( + client_request, + platform_admin_user, + webauthn_credential, + mocker +): + client_request.login(platform_admin_user) + + mocker.patch( + 'app.user_api_client.get_webauthn_credentials_for_user', + return_value=[webauthn_credential], + ) + + mock_update = mocker.patch('app.user_api_client.update_webauthn_credential_name_for_user') + + client_request.post( + 'main.user_profile_manage_security_key', + key_id=webauthn_credential['id'], + _data={'security_key_name': webauthn_credential['name']}, + _expected_status=302, + _expected_redirect=url_for( + 'main.user_profile_security_keys', + _external=True, + ) + ) + + assert not mock_update.called + + +def test_shows_delete_link_for_security_key( + mocker, + client_request, + platform_admin_user, + webauthn_credential, +): + client_request.login(platform_admin_user) + + mocker.patch( + 'app.user_api_client.get_webauthn_credentials_for_user', + return_value=[webauthn_credential], + ) + + page = client_request.get('.user_profile_manage_security_key', key_id=webauthn_credential['id']) + assert page.select_one('h1').text.strip() == f'Manage ‘{webauthn_credential["name"]}’' + + link = page.select_one('.page-footer a') + assert normalize_spaces(link.text) == 'Delete' + assert link['href'] == url_for('.user_profile_confirm_delete_security_key', key_id=webauthn_credential['id']) + + +def test_confirm_delete_security_key( + client_request, + platform_admin_user, + webauthn_credential, + mocker +): + client_request.login(platform_admin_user) + + mocker.patch( + 'app.user_api_client.get_webauthn_credentials_for_user', + return_value=[webauthn_credential], + ) + + page = client_request.get( + '.user_profile_confirm_delete_security_key', + key_id=webauthn_credential['id'], + _test_page_title=False, + ) + + assert normalize_spaces(page.select_one('.banner-dangerous').text) == ( + 'Are you sure you want to delete this security key? ' + 'Yes, delete' + ) + assert 'action' not in page.select_one('.banner-dangerous form') + assert page.select_one('.banner-dangerous form')['method'] == 'post' + + +def test_delete_security_key( + client_request, + platform_admin_user, + webauthn_credential, + mocker +): + client_request.login(platform_admin_user) + mock_delete = mocker.patch('app.user_api_client.delete_webauthn_credential_for_user') + + client_request.post( + '.user_profile_delete_security_key', + key_id=webauthn_credential['id'], + _expected_redirect=url_for( + '.user_profile_security_keys', + _external=True, + ) + ) + mock_delete.assert_called_once_with( + credential_id=webauthn_credential['id'], + user_id=platform_admin_user["id"] + ) + + +def test_delete_security_key_handles_last_credential_error( + client_request, + platform_admin_user, + webauthn_credential, + mocker, +): + client_request.login(platform_admin_user) + mocker.patch( + 'app.user_api_client.get_webauthn_credentials_for_user', + return_value=[webauthn_credential], + ) + + mocker.patch( + 'app.user_api_client.delete_webauthn_credential_for_user', + side_effect=HTTPError( + response={}, + message='Cannot delete last remaining webauthn credential for user' + ) + ) + + page = client_request.post( + '.user_profile_delete_security_key', + key_id=webauthn_credential['id'], + _follow_redirects=True + ) + assert 'Manage ‘Test credential’' in page.find('h1').text + expected_message = "You cannot delete your last security key." + assert expected_message in page.find('div', class_="banner-dangerous").text diff --git a/tests/app/test_navigation.py b/tests/app/test_navigation.py index 84a824b89..9871491c2 100644 --- a/tests/app/test_navigation.py +++ b/tests/app/test_navigation.py @@ -304,10 +304,13 @@ EXCLUDED_ENDPOINTS = tuple(map(Navigation.get_endpoint_with_blueprint, { 'usage', 'user_information', 'user_profile', + 'user_profile_confirm_delete_security_key', + 'user_profile_delete_security_key', 'user_profile_disable_platform_admin_view', 'user_profile_email', 'user_profile_email_authenticate', 'user_profile_email_confirm', + 'user_profile_manage_security_key', 'user_profile_mobile_number', 'user_profile_mobile_number_authenticate', 'user_profile_mobile_number_confirm', diff --git a/tests/conftest.py b/tests/conftest.py index 712ffbd7a..62fd5166a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4498,8 +4498,20 @@ def mock_get_invited_org_user_by_id(mocker, sample_org_invite): @pytest.fixture def webauthn_credential(): return { + 'id': str(uuid4()), 'name': 'Test credential', 'credential_data': 'WJ0AAAAAAAAAAAAAAAAAAAAAAECKU1ppjl9gmhHWyDkgHsUvZmhr6oF3/lD3llzLE2SaOSgOGIsIuAQqgp8JQSUu3r/oOaP8RS44dlQjrH+ALfYtpAECAyYhWCAxnqAfESXOYjKUc2WACuXZ3ch0JHxV0VFrrTyjyjIHXCJYIFnx8H87L4bApR4M+hPcV+fHehEOeW+KCyd0H+WGY8s6', # noqa 'registration_response': 'anything', 'created_at': '2017-10-18T16:57:14.154185Z', } + + +@pytest.fixture +def webauthn_credential_2(): + return { + 'id': str(uuid4()), + 'name': 'Another test credential', + 'credential_data': 'WJ0AAAAAAAAAAAAAAAAAAAAAAECKU1jppl9mhgHWyDkgHsUvZmhr6oF3/lD3llzLE2SaOSgOGIsIuAQqgp8JQSUu3r/oOaP8RS44dlQjrH+ALfYtpAECAyYhWCAxnqAfESXOYjKUc2WACuXZ3ch0JHxV0VFrrTyjyjIHXCJYIFnx8L4H87bApR4M+hPcV+fHehEOeW+KCyd0H+WGY8s6', # noqa + 'registration_response': 'stuff', + 'created_at': '2021-05-14T16:57:14.154185Z', + }