darkhelm/notifications-admin
1
0
Fork 0
mirror of https://github.com/GSA/notifications-admin.git synced 2026-05-26 08:09:51 -04:00
Code Issues Packages Projects Releases Wiki Activity

stop putting invite user objects in the session

Browse Source 
the invited_user objects can be arbitrarily large, and when we put them
in the session we risk going over the session cookie's 4kb size limit.
since https://github.com/alphagov/notifications-admin/pull/3827 was
merged, we store the user id in the session. Now that's been live for a
day or two we can safely stop putting the rich object in the session.

Needed to change a bunch of tests for this to make sure appropriate
mocks were set. Also some tests were accidentally re-using fake_uuid.

Still pop the object when cleaning up sessions. We'll need to remove
that in a future PR.
This commit is contained in:
Leo Hemsted
2021-03-16 17:58:27 +00:00
parent 1a63c2c8be
commit 8a2fec6f18
8 changed files with 68 additions and 167 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
app/main/views/invites.py
View File

@@ -46,7 +46,6 @@ def accept_invite(token):
return redirect(url_for('main.broadcast_tour', service_id=service.id, step_index=1))
return redirect(url_for('main.service_dashboard', service_id=invited_user.service))
session['invited_user'] = invited_user.serialize()
session['invited_user_id'] = invited_user.id
existing_user = User.from_email_address_or_none(invited_user.email_address)
@@ -108,7 +107,6 @@ def accept_org_invite(token):
session.pop('invited_org_user_id', None)
return redirect(url_for('main.organisation_dashboard', org_id=invited_org_user.organisation))
session['invited_org_user'] = invited_org_user.serialize()
session['invited_org_user_id'] = invited_org_user.id
existing_user = User.from_email_address_or_none(invited_org_user.email_address)

12
app/models/user.py
View File

@@ -522,11 +522,7 @@ class InvitedUser(JSONModel):
@classmethod
def from_session(cls):
invited_user_id = session.get('invited_user_id')
if invited_user_id:
return cls.by_id(invited_user_id)
invited_user = session.get('invited_user')
return cls(invited_user) if invited_user else None
return cls.by_id(invited_user_id) if invited_user_id else None
def has_permissions(self, *permissions):
if self.status == 'cancelled':
@@ -606,11 +602,7 @@ class InvitedOrgUser(JSONModel):
@classmethod
def from_session(cls):
invited_org_user_id = session.get('invited_org_user_id')
if invited_org_user_id:
return cls.by_id(invited_org_user_id)
invited_org_user = session.get('invited_org_user')
return cls(invited_org_user) if invited_org_user else None
return cls.by_id(invited_org_user_id) if invited_org_user_id else None
@classmethod
def by_id_and_org_id(cls, org_id, invited_user_id):

82
tests/app/main/views/organisations/test_organisation_invites.py
View File

@@ -229,11 +229,11 @@ def test_registration_from_org_invite_has_bad_data(
client,
sample_org_invite,
data,
error
error,
mock_get_invited_org_user_by_id,
):
invited_org_user = InvitedOrgUser(sample_org_invite)
with client.session_transaction() as session:
session['invited_org_user'] = invited_org_user.serialize()
session['invited_org_user_id'] = sample_org_invite['id']
response = client.post(url_for('main.register_from_org_invite'), data=data)
@@ -249,22 +249,23 @@ def test_registration_from_org_invite_has_bad_data(
def test_registration_from_org_invite_has_different_email_or_organisation(
client,
sample_org_invite,
diff_data
diff_data,
mock_get_invited_org_user_by_id,
):
invited_org_user = InvitedOrgUser(sample_org_invite)
with client.session_transaction() as session:
session['invited_org_user'] = invited_org_user.serialize()
session['invited_org_user_id'] = sample_org_invite['id']
for data in diff_data:
session['invited_org_user'][data] = 'different'
response = client.post(url_for('main.register_from_org_invite'), data={
data = {
'name': 'Test User',
'mobile_number': '+4407700900460',
'password': 'validPassword!',
'email_address': session['invited_org_user']['email_address'],
'organisation': session['invited_org_user']['organisation']
})
'email_address': sample_org_invite['email_address'],
'organisation': sample_org_invite['organisation']
}
for field in diff_data:
data[field] = 'different'
response = client.post(url_for('main.register_from_org_invite'), data=data)
assert response.status_code == 400
@@ -276,25 +277,25 @@ def test_org_user_registers_with_email_already_in_use(
mock_accept_org_invite,
mock_add_user_to_organisation,
mock_send_already_registered_email,
mock_register_user
mock_register_user,
mock_get_invited_org_user_by_id,
):
invited_org_user = InvitedOrgUser(sample_org_invite)
with client.session_transaction() as session:
session['invited_org_user'] = invited_org_user.serialize()
session['invited_org_user_id'] = sample_org_invite['id']
response = client.post(url_for('main.register_from_org_invite'), data={
'name': 'Test User',
'mobile_number': '+4407700900460',
'password': 'validPassword!',
'email_address': session['invited_org_user']['email_address'],
'organisation': session['invited_org_user']['organisation']
'email_address': sample_org_invite['email_address'],
'organisation': sample_org_invite['organisation']
})
assert response.status_code == 302
assert response.location == url_for('main.verify', _external=True)
mock_get_user_by_email.assert_called_once_with(
session['invited_org_user']['email_address']
sample_org_invite['email_address']
)
assert mock_register_user.called is False
assert mock_send_already_registered_email.called is False
@@ -310,47 +311,6 @@ def test_org_user_registration(
mock_send_verify_email,
mock_accept_org_invite,
mock_add_user_to_organisation,
):
invited_org_user = InvitedOrgUser(sample_org_invite)
with client.session_transaction() as session:
session['invited_org_user'] = invited_org_user.serialize()
response = client.post(url_for('main.register_from_org_invite'), data={
'name': 'Test User',
'email_address': session['invited_org_user']['email_address'],
'mobile_number': '+4407700900460',
'password': 'validPassword!',
'organisation': session['invited_org_user']['organisation']
})
assert response.status_code == 302
assert response.location == url_for('main.verify', _external=True)
assert mock_get_user_by_email.called is False
mock_register_user.assert_called_once_with(
'Test User',
session['invited_org_user']['email_address'],
'+4407700900460',
'validPassword!',
'sms_auth'
)
mock_send_verify_code.assert_called_once_with(
'6ce466d0-fd6a-11e5-82f5-e0accb9d11a6',
'sms',
'+4407700900460',
)
def test_org_user_registration_when_org_user_id_in_session(
client,
sample_org_invite,
mock_email_is_not_already_in_use,
mock_register_user,
mock_send_verify_code,
mock_get_user_by_email,
mock_send_verify_email,
mock_accept_org_invite,
mock_add_user_to_organisation,
mock_get_invited_org_user_by_id,
):
with client.session_transaction() as session:
@@ -368,7 +328,6 @@ def test_org_user_registration_when_org_user_id_in_session(
assert response.location == url_for('main.verify', _external=True)
assert mock_get_user_by_email.called is False
mock_get_invited_org_user_by_id.assert_called_once_with(sample_org_invite['id'])
mock_register_user.assert_called_once_with(
'Test User',
sample_org_invite['email_address'],
@@ -381,6 +340,7 @@ def test_org_user_registration_when_org_user_id_in_session(
'sms',
'+4407700900460',
)
mock_get_invited_org_user_by_id.assert_called_once_with(sample_org_invite['id'])
def test_verified_org_user_redirects_to_dashboard(

17
tests/app/main/views/test_accept_invite.py
View File

@@ -133,7 +133,7 @@ def test_invite_goes_in_session(
)
with client_request.session_transaction() as session:
assert session['invited_user']['email_address'] == 'test@user.gov.uk'
assert session['invited_user_id'] == sample_invite['id']
@pytest.mark.parametrize('user, landing_page_title', [
@@ -410,25 +410,18 @@ def test_new_user_accept_invite_completes_new_registration_redirects_to_verify(
mock_get_service,
mocker,
):
expected_service = service_one['id']
expected_email = sample_invite['email_address']
expected_from_user = service_one['users'][0]
expected_redirect_location = 'http://localhost/register-from-invite'
response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken'))
with client.session_transaction() as session:
assert response.status_code == 302
assert response.location == expected_redirect_location
invited_user = session.get('invited_user')
assert invited_user
assert 'invited_user' not in session
assert session.get('invited_user_id') == USER_ONE_ID
assert expected_service == invited_user['service']
assert expected_email == invited_user['email_address']
assert expected_from_user == invited_user['from_user']
data = {'service': invited_user['service'],
'email_address': invited_user['email_address'],
'from_user': invited_user['from_user'],
data = {'service': sample_invite['service'],
'email_address': sample_invite['email_address'],
'from_user': sample_invite['from_user'],
'password': 'longpassword',
'mobile_number': '+447890123456',
'name': 'Invited User',

97
tests/app/main/views/test_register.py
View File

@@ -1,4 +1,3 @@
from datetime import datetime
from unittest.mock import ANY
import pytest
@@ -6,7 +5,6 @@ from bs4 import BeautifulSoup
from flask import session, url_for
from flask_login import current_user
from app.models.user import InvitedUser
from tests.conftest import normalize_spaces
@@ -208,19 +206,12 @@ def test_shows_name_on_registration_page_from_invite(
fake_uuid,
email_address,
expected_value,
sample_invite,
mock_get_invited_user_by_id,
):
sample_invite['email_address'] = email_address
with client_request.session_transaction() as session:
session['invited_user'] = {
'id': fake_uuid,
'service': fake_uuid,
'from_user': "",
'email_address': email_address,
'permissions': ["manage_users"],
'status': "pending",
'created_at': datetime.utcnow(),
'auth_type': 'sms_auth',
'folder_permissions': [],
}
session['invited_user_id'] = sample_invite
page = client_request.get('main.register_from_invite')
assert page.select_one('input[name=name]').get('value') == expected_value
@@ -229,30 +220,23 @@ def test_shows_name_on_registration_page_from_invite(
def test_shows_hidden_email_address_on_registration_page_from_invite(
client_request,
fake_uuid,
sample_invite,
mock_get_invited_user_by_id,
):
with client_request.session_transaction() as session:
session['invited_user'] = {
'id': fake_uuid,
'service': fake_uuid,
'from_user': "",
'email_address': "test@example.com",
'permissions': ["manage_users"],
'status': "pending",
'created_at': datetime.utcnow(),
'auth_type': 'sms_auth',
'folder_permissions': [],
}
session['invited_user_id'] = sample_invite
page = client_request.get('main.register_from_invite')
assert normalize_spaces(page.select_one('main p').text) == (
'Your account will be created with this email address: test@example.com'
'Your account will be created with this email address: invited_user@test.gov.uk'
)
hidden_input = page.select_one('form .govuk-visually-hidden input')
for attr, value in (
('type', 'email'),
('name', 'username'),
('id', 'username'),
('value', 'test@example.com'),
('value', 'invited_user@test.gov.uk'),
('disabled', "disabled"),
('tabindex', '-1'),
('aria-hidden', 'true'),
@@ -275,30 +259,19 @@ def test_register_from_invite(
mock_register_user,
mock_send_verify_code,
mock_accept_invite,
mock_get_invited_user_by_id,
sample_invite,
extra_data,
):
invited_user = InvitedUser(
{
'id': fake_uuid,
'service': fake_uuid,
'from_user': "",
'email_address': "invited@user.com",
'permissions': ["manage_users"],
'status': "pending",
'created_at': datetime.utcnow(),
'auth_type': 'sms_auth',
'folder_permissions': [],
}
)
with client.session_transaction() as session:
session['invited_user'] = invited_user.serialize()
session['invited_user_id'] = sample_invite['id']
response = client.post(
url_for('main.register_from_invite'),
data=dict(
name='Registered in another Browser',
email_address=invited_user.email_address,
email_address=sample_invite['email_address'],
mobile_number='+4407700900460',
service=str(invited_user.id),
service=sample_invite['service'],
password='somreallyhardthingtoguess',
auth_type='sms_auth',
**extra_data
@@ -308,11 +281,12 @@ def test_register_from_invite(
assert response.location == url_for('main.verify', _external=True)
mock_register_user.assert_called_once_with(
'Registered in another Browser',
invited_user.email_address,
sample_invite['email_address'],
'+4407700900460',
'somreallyhardthingtoguess',
'sms_auth',
)
),
mock_get_invited_user_by_id.assert_called_once_with(sample_invite['id'])
def test_register_from_invite_when_user_registers_in_another_browser(
@@ -320,29 +294,19 @@ def test_register_from_invite_when_user_registers_in_another_browser(
api_user_active,
mock_get_user_by_email,
mock_accept_invite,
mock_get_invited_user_by_id,
sample_invite,
):
invited_user = InvitedUser(
{
'id': api_user_active['id'],
'service': api_user_active['id'],
'from_user': "",
'email_address': api_user_active['email_address'],
'permissions': ["manage_users"],
'status': "pending",
'created_at': datetime.utcnow(),
'auth_type': 'sms_auth',
'folder_permissions': [],
}
)
sample_invite['email_address'] = api_user_active['email_address']
with client.session_transaction() as session:
session['invited_user'] = invited_user.serialize()
session['invited_user_id'] = sample_invite['id']
response = client.post(
url_for('main.register_from_invite'),
data={
'name': 'Registered in another Browser',
'email_address': api_user_active['email_address'],
'mobile_number': api_user_active['mobile_number'],
'service': str(api_user_active['id']),
'service': sample_invite['service'],
'password': 'somreallyhardthingtoguess',
'auth_type': 'sms_auth'
}
@@ -364,6 +328,7 @@ def test_register_from_email_auth_invite(
mock_create_event,
mock_add_user_to_service,
mock_get_service,
mock_get_invited_user_by_id,
invite_email_address,
service_one,
fake_uuid,
@@ -374,7 +339,7 @@ def test_register_from_email_auth_invite(
sample_invite['auth_type'] = 'email_auth'
sample_invite['email_address'] = invite_email_address
with client.session_transaction() as session:
session['invited_user'] = sample_invite
session['invited_user_id'] = sample_invite['id']
assert not current_user.is_authenticated
data = {
@@ -401,6 +366,8 @@ def test_register_from_email_auth_invite(
data['password'],
data['auth_type']
)
# 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
@@ -412,7 +379,7 @@ def test_register_from_email_auth_invite(
with client.session_transaction() as session:
# invited user details are still there so they can get added to the service
assert session['invited_user'] == sample_invite
assert session['invited_user_id'] == sample_invite['id']
def test_can_register_email_auth_without_phone_number(
@@ -427,10 +394,11 @@ def test_can_register_email_auth_without_phone_number(
mock_create_event,
mock_add_user_to_service,
mock_get_service,
mock_get_invited_user_by_id,
):
sample_invite['auth_type'] = 'email_auth'
with client.session_transaction() as session:
session['invited_user'] = sample_invite
session['invited_user_id'] = sample_invite['id']
data = {
'name': 'invited user',
@@ -474,11 +442,12 @@ def test_cannot_register_with_sms_auth_and_missing_mobile_number(
def test_register_from_invite_form_doesnt_show_mobile_number_field_if_email_auth(
client,
sample_invite
sample_invite,
mock_get_invited_user_by_id,
):
sample_invite['auth_type'] = 'email_auth'
with client.session_transaction() as session:
session['invited_user'] = sample_invite
session['invited_user_id'] = sample_invite['id']
response = client.get(url_for('main.register_from_invite'))

6
tests/app/main/views/test_sign_in.py
View File

@@ -224,7 +224,8 @@ def test_email_address_is_treated_case_insensitively_when_signing_in_as_invited_
api_user_active,
sample_invite,
mock_accept_invite,
mock_send_verify_code
mock_send_verify_code,
mock_get_invited_user_by_id,
):
sample_invite['email_address'] = 'TEST@user.gov.uk'
@@ -234,7 +235,7 @@ def test_email_address_is_treated_case_insensitively_when_signing_in_as_invited_
)
with client.session_transaction() as session:
session['invited_user'] = sample_invite
session['invited_user_id'] = sample_invite['id']
response = client.post(
url_for('main.sign_in'), data={
@@ -244,3 +245,4 @@ def test_email_address_is_treated_case_insensitively_when_signing_in_as_invited_
assert mock_accept_invite.called
assert response.status_code == 302
assert mock_send_verify_code.called
mock_get_invited_user_by_id.assert_called_once_with(sample_invite['id'])

5
tests/app/main/views/test_verify.py
View File

@@ -176,6 +176,7 @@ def test_activate_user_redirects_to_service_dashboard_if_user_already_belongs_to
api_user_active,
mock_login,
mock_get_service,
mock_get_invited_user_by_id,
):
mocker.patch('app.user_api_client.add_user_to_service', side_effect=HTTPError(
response=Mock(
@@ -189,10 +190,10 @@ def test_activate_user_redirects_to_service_dashboard_if_user_already_belongs_to
))
# Can't use `with client.session_transaction()...` here since activate_session is not a view function
flask_session['invited_user'] = sample_invite
flask_session['invited_user_id'] = sample_invite['id']
response = activate_user(api_user_active['id'])
assert response.location == url_for('main.service_dashboard', service_id=service_one['id'])
flask_session.pop('invited_user')
flask_session.pop('invited_user_id')

14
tests/app/models/test_user.py
View File

@@ -137,13 +137,6 @@ def test_invited_user_from_session_uses_id_even_if_obj_in_session(
mock_get_invited_user_by_id.assert_called_once_with(USER_ONE_ID)
def test_invited_user_from_session_uses_obj_if_id_not_present(client, mocker, sample_invite):
session_dict = {'invited_user': sample_invite}
mocker.patch.dict('app.models.user.session', values=session_dict, clear=True)
assert InvitedUser.from_session().id == USER_ONE_ID
def test_invited_user_from_session_returns_none_if_nothing_present(client, mocker):
mocker.patch.dict('app.models.user.session', values={}, clear=True)
assert InvitedUser.from_session() is None
@@ -176,13 +169,6 @@ def test_invited_org_user_from_session_uses_id_even_if_obj_in_session(
mock_get_invited_org_user_by_id.assert_called_once_with(fake_id)
def test_invited_org_user_from_session_uses_obj_if_id_not_present(client, mocker, sample_org_invite):
session_dict = {'invited_org_user': sample_org_invite}
mocker.patch.dict('app.models.user.session', values=session_dict, clear=True)
assert InvitedOrgUser.from_session().id == sample_org_invite['id']
def test_invited_org_user_from_session_returns_none_if_nothing_present(client, mocker):
mocker.patch.dict('app.models.user.session', values={}, clear=True)
assert InvitedOrgUser.from_session() is None