From c2d9a56ff43af702672a38a4b7646834e8c6d82f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 11 Oct 2021 17:41:23 +0100 Subject: [PATCH 1/2] Bump Werkzeug to version 2.0.2 This is the newest version. Pyup is complaining about vulnerabilities in version 1.0.1, specifically > Werkzeug version 2.0.2 improves the security of the debugger cookies. > "SameSite" attribute is set to "Strict" instead of "None", and the > secure flag is added when on HTTPS. Previously we were using whatever version of Werkzeug that Flask specified this pins it to get rid of the vulnerability without having to upgrade everything at once. This requires a few changes to tests which were relying on importing `session` and `current_user` from Flask. Previously it seemed that importing these in the tests referred to the same object that was being used in the app. This appears to no longer be the case. This commit works around that by: - using a context manager to get the contents of the session, like we already do in most tests - asserting that the mock which logs the user in is being called with the right values, rather than looking at the state of the `current_user` object (which was probably giving false certainty anyway) --- requirements.in | 1 + requirements.txt | 8 ++++++-- tests/app/main/views/test_add_service.py | 8 +++++--- tests/app/main/views/test_register.py | 22 ++++++++++++++++------ tests/app/main/views/test_sign_out.py | 10 ++++++---- 5 files changed, 34 insertions(+), 15 deletions(-) diff --git a/requirements.in b/requirements.in index f3868b299..44ee5c1d7 100644 --- a/requirements.in +++ b/requirements.in @@ -7,6 +7,7 @@ humanize==3.6.0 Flask==1.1.2 # pyup: <2 Flask-WTF==0.15.1 Flask-Login==0.5.0 +werkzeug==2.0.2 blinker==1.4 pyexcel==0.6.6 diff --git a/requirements.txt b/requirements.txt index 941268c3e..399ce628c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -41,6 +41,8 @@ cryptography==3.3.2 # via # -r requirements.in # fido2 +dataclasses==0.8 + # via werkzeug dnspython==1.16.0 # via eventlet docopt==0.6.2 @@ -216,8 +218,10 @@ urllib3==1.26.5 # requests webencodings==0.5.1 # via bleach -werkzeug==1.0.1 - # via flask +werkzeug==2.0.2 + # via + # -r requirements.in + # flask wtforms==2.3.3 # via flask-wtf xlrd==1.2.0 diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index 82b943702..4e8c35ac2 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -1,5 +1,5 @@ import pytest -from flask import session, url_for +from flask import url_for from freezegun import freeze_time from notifications_python_client.errors import HTTPError @@ -170,7 +170,8 @@ def test_should_add_service_and_redirect_to_tour_when_no_services( ), 101, ) - assert session['service_id'] == 101 + with client_request.session_transaction() as session: + assert session['service_id'] == 101 def test_add_service_has_to_choose_org_type( @@ -283,7 +284,8 @@ def test_should_add_service_and_redirect_to_dashboard_when_existing_service( email_from='testing.the.post', ) assert len(mock_create_service_template.call_args_list) == 0 - assert session['service_id'] == 101 + with client_request.session_transaction() as session: + assert session['service_id'] == 101 @pytest.mark.parametrize('name, error_message', [ diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index 74e42743d..97a3bf0cf 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -2,9 +2,9 @@ from unittest.mock import ANY import pytest from bs4 import BeautifulSoup -from flask import session, url_for -from flask_login import current_user +from flask import url_for +from app.models.user import User from tests.conftest import normalize_spaces @@ -145,7 +145,8 @@ def test_should_add_user_details_to_session( }, ) assert response.status_code == 302 - assert session['user_details']['email'] == email_address + with client.session_transaction() as session: + assert session['user_details']['email'] == email_address def test_should_return_200_if_password_is_on_list_of_commonly_used_passwords( @@ -334,11 +335,11 @@ def test_register_from_email_auth_invite( fake_uuid, mocker, ): + mock_login_user = mocker.patch('app.models.user.login_user') sample_invite['auth_type'] = 'email_auth' sample_invite['email_address'] = invite_email_address with client.session_transaction() as session: session['invited_user_id'] = sample_invite['id'] - assert not current_user.is_authenticated data = { 'name': 'invited user', @@ -367,9 +368,18 @@ def test_register_from_email_auth_invite( # this is actually called twice, at the beginning of the function and then by the activate_user function mock_get_invited_user_by_id.assert_called_with(sample_invite['id']) mock_accept_invite.assert_called_once_with(sample_invite['service'], sample_invite['id']) + # just logs them in - assert current_user.is_authenticated - assert mock_add_user_to_service.called + mock_login_user.assert_called_once_with(User({ + 'id': fake_uuid, # This ID matches the return value of mock_register_user + 'platform_admin': False + })) + mock_add_user_to_service.assert_called_once_with( + sample_invite['service'], + fake_uuid, # This ID matches the return value of mock_register_user + {'manage_api_keys', 'manage_service', 'send_messages', 'view_activity'}, + [], + ) with client.session_transaction() as session: # invited user details are still there so they can get added to the service diff --git a/tests/app/main/views/test_sign_out.py b/tests/app/main/views/test_sign_out.py index ed5929d3f..217fb3a35 100644 --- a/tests/app/main/views/test_sign_out.py +++ b/tests/app/main/views/test_sign_out.py @@ -1,4 +1,3 @@ -import flask from flask import url_for from tests.conftest import SERVICE_ONE_ID @@ -7,13 +6,15 @@ from tests.conftest import SERVICE_ONE_ID def test_render_sign_out_redirects_to_sign_in( logged_in_client_with_session ): - assert flask.session + with logged_in_client_with_session.session_transaction() as session: + assert session response = logged_in_client_with_session.get( url_for('main.sign_out')) assert response.status_code == 302 assert response.location == url_for( 'main.index', _external=True) - assert not flask.session + with logged_in_client_with_session.session_transaction() as session: + assert not session def test_sign_out_user( @@ -57,7 +58,8 @@ def test_sign_out_of_two_sessions( ): logged_in_client_with_session.get( url_for('main.sign_out')) - assert not flask.session + with logged_in_client_with_session.session_transaction() as session: + assert not session response = logged_in_client_with_session.get( url_for('main.sign_out')) From c3bbc427e2cce3c646ae04c42f388b1cc5744627 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 13 Oct 2021 11:31:27 +0100 Subject: [PATCH 2/2] =?UTF-8?q?Check=20that=20user=20isn=E2=80=99t=20signe?= =?UTF-8?q?d=20in=20before=20registering?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously this test asserted on `current_user.is_authenticated`. That isn’t possible now because the object imported into tests isn’t the same one the app is using. A different proxy for whether the user is signed in is whether they have a user id in their session, because we set this every time they sign in: https://github.com/alphagov/notifications-admin/blob/ff32e73d9bda80e556a9ee81fb0d867eef35e1e2/app/models/user.py#L162 --- tests/app/main/views/test_register.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index 97a3bf0cf..f8c974148 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -340,6 +340,8 @@ def test_register_from_email_auth_invite( sample_invite['email_address'] = invite_email_address with client.session_transaction() as session: session['invited_user_id'] = sample_invite['id'] + # Prove that the user isn’t already signed in + assert 'user_id' not in session data = { 'name': 'invited user', @@ -382,6 +384,8 @@ def test_register_from_email_auth_invite( ) with client.session_transaction() as session: + # The user is signed in + assert 'user_id' in session # invited user details are still there so they can get added to the service assert session['invited_user_id'] == sample_invite['id']