From d05f127e415b2b6eeed2c08d9ecfa3971962674e Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 25 May 2021 15:50:14 +0100 Subject: [PATCH] return 200 to js instead of 302 when logging in the js fetch function is really not designed to work with 302s. when it receives a 302, it automatically follows it and fetches the next page. This is awkward because I don't want js to do all this in ajax, I want the browser to get the new URL so it can load the page. A better approach is to view the admin endpoint as a more pure API: the js sends a request for authentication to the admin app, and the admin app responds with a 200 indicating success, and then a payload of relevant data with that. The relevant data in this case is "Which URL should I redirect to", it might be the user's list of services page, or it might be a page telling them that their email needs revalidating. --- .../javascripts/authenticateSecurityKey.js | 21 ++++++++++--------- app/main/views/webauthn_credentials.py | 3 ++- .../main/views/test_webauthn_credentials.py | 20 ++++++++---------- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/app/assets/javascripts/authenticateSecurityKey.js b/app/assets/javascripts/authenticateSecurityKey.js index 5f604962b..d9f396580 100644 --- a/app/assets/javascripts/authenticateSecurityKey.js +++ b/app/assets/javascripts/authenticateSecurityKey.js @@ -33,21 +33,22 @@ }); }) .then(response => { - if (response.status === 403){ + if (response.status === 403) { // flask will have `flash`ed an error message up window.location.reload(); return; } - if (!response.ok) { - // probably an internal server error - throw Error(response.statusText); - } - - // fetch will already have done the login redirect dance and will at this point be - // referring to the final 200 - hopefully to the `/accounts` url or similar. Set the location - // to trigger a browser navigate to that URL. - window.location.href = response.url; + return response.arrayBuffer() + .then(cbor => { + return Promise.resolve(window.CBOR.decode(cbor)); + }) + .catch(() => { + throw Error(response.statusText); + }) + .then(data => { + window.location = data.redirect_url; + }); }) .catch(error => { console.error(error); diff --git a/app/main/views/webauthn_credentials.py b/app/main/views/webauthn_credentials.py index 5fe4c5435..2bfdf98ac 100644 --- a/app/main/views/webauthn_credentials.py +++ b/app/main/views/webauthn_credentials.py @@ -97,7 +97,8 @@ def webauthn_complete_authentication(): _complete_webauthn_authentication(user_to_login) - return _verify_webauthn_login(user_to_login) + redirect = _verify_webauthn_login(user_to_login) + return cbor.encode({'redirect_url': redirect.location}), 200 def _complete_webauthn_authentication(user): diff --git a/tests/app/main/views/test_webauthn_credentials.py b/tests/app/main/views/test_webauthn_credentials.py index da8def9eb..54e1254cf 100644 --- a/tests/app/main/views/test_webauthn_credentials.py +++ b/tests/app/main/views/test_webauthn_credentials.py @@ -1,5 +1,5 @@ import base64 -from unittest.mock import ANY +from unittest.mock import ANY, Mock import pytest from fido2 import cbor @@ -260,13 +260,12 @@ def test_complete_authentication_checks_credentials( platform_admin_user['auth_type'] = 'webauthn_auth' mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[webauthn_credential]) - # fake returning a 200 just to keep flask happy, normally this'll redirect - mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_login', return_value=('ok', 200)) + mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_login', return_value=Mock(location='/foo')) response = client.post(url_for('main.webauthn_complete_authentication'), data=webauthn_authentication_post_data) - # matches response of verify_webauthn_login - assert response.data == b'ok' + assert response.status_code == 200 + assert cbor.decode(response.data) == {'redirect_url': '/foo'} def test_complete_authentication_403s_if_key_isnt_in_users_credentials( @@ -311,8 +310,7 @@ def test_complete_authentication_clears_session( platform_admin_user['auth_type'] = 'webauthn_auth' mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) mocker.patch('app.user_api_client.get_webauthn_credentials_for_user', return_value=[webauthn_credential]) - # fake returning a 200 just to keep flask happy, normally this'll redirect - mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_login', return_value=('ok', 200)) + mocker.patch('app.main.views.webauthn_credentials._verify_webauthn_login', return_value=Mock(location='/foo')) client.post(url_for('main.webauthn_complete_authentication'), data=webauthn_authentication_post_data) @@ -337,8 +335,8 @@ def test_verify_webauthn_login_signs_user_in_signs_user_in(client, mocker, mock_ resp = client.post(url_for('main.webauthn_complete_authentication')) - assert resp.status_code == 302 - assert resp.location == url_for('main.show_accounts_or_dashboard', _external=True) + assert resp.status_code == 200 + assert cbor.decode(resp.data)['redirect_url'] == url_for('main.show_accounts_or_dashboard') # removes stuff from session with client.session_transaction() as session: assert 'user_details' not in session @@ -390,8 +388,8 @@ def test_verify_webauthn_login_signs_user_in_sends_revalidation_email_if_needed( resp = client.post(url_for('main.webauthn_complete_authentication')) - assert resp.status_code == 302 - assert resp.location == url_for('main.revalidate_email_sent', _external=True) + assert resp.status_code == 200 + assert cbor.decode(resp.data)['redirect_url'] == url_for('main.revalidate_email_sent') with client.session_transaction() as session: # stuff stays in session so we can log them in later when they validate their email