Merge pull request #3926 from alphagov/sign-in-bug

ensure user details are always in the session after entering password

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(

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.

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', [
@@ -274,3 +279,39 @@ def test_email_address_is_treated_case_insensitively_when_signing_in_as_invited_
     assert response.status_code == 302
     assert mock_send_verify_code.called
     mock_get_invited_user_by_id.assert_called_once_with(sample_invite['id'])
+
+
+def test_when_signing_in_as_invited_user_you_cannot_accept_an_invite_for_another_email_address(
+    client_request,
+    mocker,
+    mock_verify_password,
+    api_user_active,
+    sample_invite,
+    mock_accept_invite,
+    mock_send_verify_code,
+    mock_get_invited_user_by_id,
+):
+    sample_invite['email_address'] = 'some_other_user@user.gov.uk'
+
+    mocker.patch(
+        'app.models.user.User.from_email_address_and_password_or_none',
+        return_value=User(api_user_active),
+    )
+
+    client_request.logout()
+
+    with client_request.session_transaction() as session:
+        session['invited_user_id'] = sample_invite['id']
+
+    page = client_request.post(
+        'main.sign_in',
+        _data={
+            'email_address': 'test@user.gov.uk',
+            'password': 'val1dPassw0rd!'
+        },
+        _expected_status=403
+    )
+
+    assert mock_accept_invite.called is False
+    assert mock_send_verify_code.called is False
+    assert page.select_one('.banner-dangerous').text.strip() == 'You cannot accept an invite for another person.'