From a946ad6ec273b399e798f68c9906851a43bff60c Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 14 May 2021 18:42:57 +0100 Subject: [PATCH] Let admin user delete their security key Show confiem delete dialogue first to confirm if key should be deleted. --- app/main/views/user_profile.py | 26 +++++- app/notify_client/user_api_client.py | 5 ++ .../user-profile/manage-security-key.html | 7 +- tests/app/main/views/test_user_profile.py | 79 ++++++++++++++++++- tests/app/test_navigation.py | 2 + 5 files changed, 116 insertions(+), 3 deletions(-) diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index a19ee96bb..a53d0f76f 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -3,8 +3,10 @@ import json from flask import ( abort, current_app, + flash, redirect, render_template, + request, session, url_for, ) @@ -240,7 +242,16 @@ def user_profile_security_keys(): ) -@main.route("/user-profile/security-keys//manage", methods=['GET', 'POST']) +@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 = user_api_client.get_webauthn_credentials_for_user(current_user.id) @@ -259,8 +270,21 @@ def user_profile_manage_security_key(key_id): ) 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): + user_api_client.delete_webauthn_credential_for_user( + user_id=current_user.id, + credential_id=key_id + ) + 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 5b54a785d..3909dbbea 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -206,5 +206,10 @@ class UserApiClient(NotifyAdminAPIClient): 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 index 7c5030c9c..7e21f2202 100644 --- a/app/templates/views/user-profile/manage-security-key.html +++ b/app/templates/views/user-profile/manage-security-key.html @@ -20,7 +20,12 @@
{% call form_wrapper(autocomplete=True) %} {{ form.name_of_key }} - {{ page_footer('Save') }} + {{ page_footer( + 'Save', + delete_link=url_for( + '.user_profile_confirm_delete_security_key', key_id=security_key.id), + delete_link_text='Delete' + ) }} {% endcall %}
diff --git a/tests/app/main/views/test_user_profile.py b/tests/app/main/views/test_user_profile.py index 226cf46e6..97300d072 100644 --- a/tests/app/main/views/test_user_profile.py +++ b/tests/app/main/views/test_user_profile.py @@ -5,7 +5,11 @@ import pytest from flask import url_for from notifications_utils.url_safe_token import generate_token -from tests.conftest import create_api_user_active, url_for_endpoint_with_token +from tests.conftest import ( + create_api_user_active, + normalize_spaces, + url_for_endpoint_with_token, +) def test_should_show_overview_page( @@ -462,3 +466,76 @@ def test_should_redirect_after_change_of_security_key_name( new_name_for_credential="new name", user_id=platform_admin_user["id"] ) + + +def test_shows_delete_link_for_security_key( + 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, webauthn_credential_2], + ) + + 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, + webauthn_credential_2, + mocker +): + client_request.login(platform_admin_user) + + mocker.patch( + 'app.user_api_client.get_webauthn_credentials_for_user', + return_value=[webauthn_credential, webauthn_credential_2], + ) + + 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"] + ) diff --git a/tests/app/test_navigation.py b/tests/app/test_navigation.py index f91142383..1e51a901b 100644 --- a/tests/app/test_navigation.py +++ b/tests/app/test_navigation.py @@ -303,6 +303,8 @@ 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',