From 7b3751240c747047af66f4581c1530c5a877ccdb Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 10 Jun 2021 19:07:35 +0100 Subject: [PATCH] ensure user details are always in the session after entering password We signal that we're mid-way through the sign-in flow by adding a `user_details` dict to the session. previously, we'd only put a user's details in the session in `User.sign_in`, just before sending any 2fa prompt and redirecting to the two factor pages. However, we found a bug where a user with no session (eg, using a fresh browser) tried to log in, but they had never clicked the link to validate their email address when registering. Their user's state was still in "pending", so we redirected to `main.resend_email_verification` as intended - however, they didn't have anything in the session and the resend page expected to get the email address to resend to out of that. To be safe, as soon as we've confirmed the user has entered their password correctly, lets save the session data at that point. That way any redirects will be fine. --- app/main/views/sign_in.py | 41 ++++++++++++++++------------ app/models/user.py | 10 +------ tests/app/main/views/test_sign_in.py | 19 ++++++++----- 3 files changed, 37 insertions(+), 33 deletions(-) diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index a119faada..f313c0855 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -33,24 +33,31 @@ def sign_in(): form.email_address.data, form.password.data ) - if user and user.state == 'pending': - return redirect(url_for('main.resend_email_verification', next=redirect_url)) + if user: + # add user to session to mark us as in the process of signing the user in + session['user_details'] = {"email": user.email_address, "id": user.id} - if user and session.get('invited_user_id'): - invited_user = InvitedUser.from_session() - if user.email_address.lower() != invited_user.email_address.lower(): - flash("You cannot accept an invite for another person.") - session.pop('invited_user_id', None) - abort(403) - else: - invited_user.accept_invite() - if user and user.sign_in(): - if user.sms_auth: - return redirect(url_for('.two_factor_sms', next=redirect_url)) - if user.email_auth: - return redirect(url_for('.two_factor_email_sent', next=redirect_url)) - if user.webauthn_auth: - return redirect(url_for('.two_factor_webauthn', next=redirect_url)) + if user.state == 'pending': + return redirect(url_for('main.resend_email_verification', next=redirect_url)) + + if user.is_active: + if session.get('invited_user_id'): + invited_user = InvitedUser.from_session() + if user.email_address.lower() != invited_user.email_address.lower(): + flash("You cannot accept an invite for another person.") + session.pop('invited_user_id', None) + abort(403) + else: + invited_user.accept_invite() + + user.send_login_code() + + if user.sms_auth: + return redirect(url_for('.two_factor_sms', next=redirect_url)) + if user.email_auth: + return redirect(url_for('.two_factor_email_sent', next=redirect_url)) + if user.webauthn_auth: + return redirect(url_for('.two_factor_webauthn', next=redirect_url)) # Vague error message for login in case of user not known, locked, inactive or password not verified flash(Markup( diff --git a/app/models/user.py b/app/models/user.py index 0d95e1624..e52661b2b 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -142,20 +142,12 @@ class User(JSONModel, UserMixin): login_user(self) session['user_id'] = self.id - def sign_in(self): - - session['user_details'] = {"email": self.email_address, "id": self.id} - - if not self.is_active: - return False - + def send_login_code(self): if self.email_auth: user_api_client.send_verify_code(self.id, 'email', None, request.args.get('next')) if self.sms_auth: user_api_client.send_verify_code(self.id, 'sms', self.mobile_number) - return True - def sign_out(self): session.clear() # Update the db so the server also knows the user is logged out. diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index a96fa0ad5..c177ca974 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -1,7 +1,6 @@ import uuid import pytest -from bs4 import BeautifulSoup from flask import url_for from app.models.user import User @@ -215,16 +214,22 @@ def test_should_return_200_when_user_does_not_exist( def test_should_return_redirect_when_user_is_pending( client, mock_get_user_by_email_pending, + api_user_pending, mock_verify_password, ): response = client.post( - url_for('main.sign_in'), data={ + url_for('main.sign_in'), + data={ 'email_address': 'pending_user@example.gov.uk', - 'password': 'val1dPassw0rd!'}, follow_redirects=True) - - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.string == 'Sign in' - assert response.status_code == 200 + 'password': 'val1dPassw0rd!' + } + ) + assert response.location == url_for('main.resend_email_verification', _external=True) + with client.session_transaction() as s: + assert s['user_details'] == { + 'email': api_user_pending['email_address'], + 'id': api_user_pending['id'] + } @pytest.mark.parametrize('redirect_url', [