From 04d1d97d4cbe72f37cf392bfc52a57bc3882d1a3 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 25 May 2021 11:15:57 +0100 Subject: [PATCH] Refactor loop to separate function and use user model when getting a list of security keys Also test separately that we are correctly choosing key out of list of security keys. Previously we have done it as a part of testing pages where where we were calling API to get a list of keys, but then choosing one of those keys based on id. Also remove redundant second test credential after PR review Also remove redundant return value from mocks in update name tests --- app/main/views/user_profile.py | 12 ++++++--- tests/app/main/views/test_user_profile.py | 32 ++++++++++------------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index 23106bb9e..f6c701874 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -243,6 +243,10 @@ def user_profile_security_keys(): ) +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'], @@ -255,16 +259,16 @@ def user_profile_security_keys(): ) @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) - security_key = next((key for key in security_keys if key["id"] == key_id), None) + 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"]) + form = ChangeSecurityKeyNameForm(security_key_name=security_key.name) if form.validate_on_submit(): - if form.security_key_name.data != security_key["name"]: + 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, diff --git a/tests/app/main/views/test_user_profile.py b/tests/app/main/views/test_user_profile.py index a153d43ce..37e16634b 100644 --- a/tests/app/main/views/test_user_profile.py +++ b/tests/app/main/views/test_user_profile.py @@ -6,6 +6,8 @@ from flask import url_for from notifications_python_client.errors import HTTPError from notifications_utils.url_safe_token import generate_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, @@ -381,18 +383,22 @@ def test_should_show_security_keys_page( 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, - 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], + return_value=[webauthn_credential], ) page = client_request.get('.user_profile_manage_security_key', key_id=webauthn_credential['id']) @@ -453,21 +459,17 @@ def test_should_redirect_after_change_of_security_key_name( 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], - ) - - mock_update = mocker.patch( - 'app.user_api_client.update_webauthn_credential_name_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'], @@ -490,21 +492,17 @@ def test_user_profile_manage_security_key_should_not_call_api_if_key_name_stays_ 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], - ) - - mock_update = mocker.patch( - 'app.user_api_client.update_webauthn_credential_name_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'], @@ -524,13 +522,12 @@ def test_shows_delete_link_for_security_key( 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], + return_value=[webauthn_credential], ) page = client_request.get('.user_profile_manage_security_key', key_id=webauthn_credential['id']) @@ -545,14 +542,13 @@ 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], + return_value=[webauthn_credential], ) page = client_request.get(