From 79393c97efe56a9470ae36f7ae497aab14cb1a23 Mon Sep 17 00:00:00 2001 From: chrisw Date: Fri, 10 Nov 2017 12:35:21 +0000 Subject: [PATCH 1/7] Updated invite email auth user flow --- app/main/forms.py | 19 +++++++++++++++---- app/main/views/register.py | 12 +++++++++--- app/main/views/verify.py | 16 ++++++++++------ 3 files changed, 34 insertions(+), 13 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 5c3c389ca..d5e680309 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -109,7 +109,8 @@ class UKMobileNumber(TelField): class InternationalPhoneNumber(TelField): def pre_validate(self, form): try: - validate_phone_number(self.data, international=True) + if self.data: + validate_phone_number(self.data, international=True) except InvalidPhoneError as e: raise ValidationError(str(e)) @@ -173,13 +174,23 @@ class RegisterUserForm(Form): class RegisterUserFromInviteForm(Form): - name = StringField('Full name', - validators=[DataRequired(message='Can’t be empty')]) - mobile_number = international_phone_number() + def __init__(self, auth_type, *args, **kwargs): + self.auth_type = auth_type + super().__init__(*args, **kwargs) + + name = StringField( + 'Full name', + validators=[DataRequired(message='Can’t be empty')] + ) + mobile_number = InternationalPhoneNumber('Mobile number', validators=[]) password = password() service = HiddenField('service') email_address = HiddenField('email_address') + def validate_mobile_number(self, field): + if self.auth_type == 'sms_auth' and not field.data: + raise ValidationError('Can’t be empty') + class PermissionsForm(Form): send_messages = BooleanField("Send messages from existing templates") diff --git a/app/main/views/register.py b/app/main/views/register.py index 5e7f9d062..523a65cc2 100644 --- a/app/main/views/register.py +++ b/app/main/views/register.py @@ -19,6 +19,7 @@ from app.main.forms import ( RegisterUserForm, RegisterUserFromInviteForm ) +from app.main.views.verify import activate_user from app import ( user_api_client, @@ -41,17 +42,22 @@ def register(): @main.route('/register-from-invite', methods=['GET', 'POST']) def register_from_invite(): - form = RegisterUserFromInviteForm() invited_user = session.get('invited_user') + form = RegisterUserFromInviteForm(invited_user['auth_type']) if not invited_user: abort(404) if form.validate_on_submit(): if form.service.data != invited_user['service'] or form.email_address.data != invited_user['email_address']: abort(400) - _do_registration(form, send_email=False) + _do_registration(form, send_email=False, send_sms=invited_user['auth_type'] == 'sms_auth') invite_api_client.accept_invite(invited_user['service'], invited_user['id']) - return redirect(url_for('main.verify')) + if invited_user['auth_type'] == 'sms_auth': + return redirect(url_for('main.verify')) + else: + # we've already proven this user has email because they clicked the invite link, + # so just activate them straight away + return activate_user(session['user_details']['id']) form.service.data = invited_user['service'] form.email_address.data = invited_user['email_address'] diff --git a/app/main/views/verify.py b/app/main/views/verify.py index e94af163e..1b3bd70fb 100644 --- a/app/main/views/verify.py +++ b/app/main/views/verify.py @@ -35,12 +35,7 @@ def verify(): if form.validate_on_submit(): try: - user = user_api_client.get_user(user_id) - # the user will have a new current_session_id set by the API - store it in the cookie for future requests - session['current_session_id'] = user.current_session_id - activated_user = user_api_client.activate_user(user) - login_user(activated_user) - return redirect(url_for('main.add_service', first='first')) + return activate_user(user_id) finally: session.pop('user_details', None) @@ -73,3 +68,12 @@ def verify_email(token): session['user_details'] = {"email": user.email_address, "id": user.id} user_api_client.send_verify_code(user.id, 'sms', user.mobile_number) return redirect('verify') + + +def activate_user(user_id): + user = user_api_client.get_user(user_id) + # the user will have a new current_session_id set by the API - store it in the cookie for future requests + session['current_session_id'] = user.current_session_id + activated_user = user_api_client.activate_user(user) + login_user(activated_user) + return redirect(url_for('main.add_service', first='first')) From 65ba7e88c811963309bc4bde9b969efb5b457042 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 13 Nov 2017 13:39:31 +0000 Subject: [PATCH 2/7] refactor RegisterFromInvite to make auth_type required, and update test fixtures --- app/main/forms.py | 9 ++- app/main/views/register.py | 5 +- app/notify_client/models.py | 2 +- app/notify_client/user_api_client.py | 5 +- tests/__init__.py | 20 ++++--- tests/app/main/views/test_accept_invite.py | 65 ++-------------------- tests/app/main/views/test_register.py | 46 +++++++++------ tests/conftest.py | 7 ++- 8 files changed, 61 insertions(+), 98 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index d5e680309..2363b7e28 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -171,13 +171,11 @@ class RegisterUserForm(Form): email_address = email_address() mobile_number = international_phone_number() password = password() + # always register as sms type + auth_type = HiddenField('auth_type', default='sms_auth') class RegisterUserFromInviteForm(Form): - def __init__(self, auth_type, *args, **kwargs): - self.auth_type = auth_type - super().__init__(*args, **kwargs) - name = StringField( 'Full name', validators=[DataRequired(message='Can’t be empty')] @@ -186,9 +184,10 @@ class RegisterUserFromInviteForm(Form): password = password() service = HiddenField('service') email_address = HiddenField('email_address') + auth_type = HiddenField('auth_type', validators=[DataRequired()]) def validate_mobile_number(self, field): - if self.auth_type == 'sms_auth' and not field.data: + if self.auth_type.data == 'sms_auth' and not field.data: raise ValidationError('Can’t be empty') diff --git a/app/main/views/register.py b/app/main/views/register.py index 523a65cc2..9e92fa444 100644 --- a/app/main/views/register.py +++ b/app/main/views/register.py @@ -43,7 +43,7 @@ def register(): @main.route('/register-from-invite', methods=['GET', 'POST']) def register_from_invite(): invited_user = session.get('invited_user') - form = RegisterUserFromInviteForm(invited_user['auth_type']) + form = RegisterUserFromInviteForm() if not invited_user: abort(404) @@ -70,7 +70,8 @@ def _do_registration(form, service=None, send_sms=True, send_email=True): user = user_api_client.register_user(form.name.data, form.email_address.data, form.mobile_number.data, - form.password.data) + form.password.data, + form.auth_type.data) # TODO possibly there should be some exception handling # for sending sms and email codes. diff --git a/app/notify_client/models.py b/app/notify_client/models.py index fe9775a36..1be63849b 100644 --- a/app/notify_client/models.py +++ b/app/notify_client/models.py @@ -150,7 +150,7 @@ class User(UserMixin): class InvitedUser(object): - def __init__(self, id, service, from_user, email_address, permissions, status, created_at, auth_type=None): + def __init__(self, id, service, from_user, email_address, permissions, status, created_at, auth_type): self.id = id self.service = str(service) self.from_user = from_user diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 2579b5e5b..272e19e11 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -21,12 +21,13 @@ class UserApiClient(NotifyAdminAPIClient): self.api_key = app.config['ADMIN_CLIENT_SECRET'] self.max_failed_login_count = app.config["MAX_FAILED_LOGIN_COUNT"] - def register_user(self, name, email_address, mobile_number, password): + def register_user(self, name, email_address, mobile_number, password, auth_type): data = { "name": name, "email_address": email_address, "mobile_number": mobile_number, - "password": password + "password": password, + "auth_type": auth_type } user_data = self.post("/user", data) return User(user_data['data'], max_failed_login_count=self.max_failed_login_count) diff --git a/tests/__init__.py b/tests/__init__.py index f59633f55..c8f7ec629 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -149,15 +149,17 @@ def api_key_json(id_, name, expiry_date=None): } -def invite_json(id_, from_user, service_id, email_address, permissions, created_at, status): - return {'id': id_, - 'from_user': from_user, - 'service': service_id, - 'email_address': email_address, - 'status': status, - 'permissions': permissions, - 'created_at': created_at - } +def invite_json(id_, from_user, service_id, email_address, permissions, created_at, status, auth_type): + return { + 'id': id_, + 'from_user': from_user, + 'service': service_id, + 'email_address': email_address, + 'status': status, + 'permissions': permissions, + 'created_at': created_at, + 'auth_type': auth_type + } TEST_USER_EMAIL = 'test@user.gov.uk' diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 913ff09d6..6c6ec4abc 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -267,7 +267,8 @@ def test_new_user_accept_invite_completes_new_registration_redirects_to_verify( 'from_user': invited_user['from_user'], 'password': 'longpassword', 'mobile_number': '+447890123456', - 'name': 'Invited User' + 'name': 'Invited User', + 'auth_type': 'email_auth' } expected_redirect_location = 'http://localhost/verify' @@ -280,7 +281,8 @@ def test_new_user_accept_invite_completes_new_registration_redirects_to_verify( mock_register_user.assert_called_with(data['name'], data['email_address'], data['mobile_number'], - data['password']) + data['password'], + data['auth_type']) assert mock_accept_invite.call_count == 1 @@ -312,65 +314,6 @@ def test_signed_in_existing_user_cannot_use_anothers_invite( assert mock_accept_invite.call_count == 0 -def test_new_invited_user_verifies_and_added_to_service( - client, - service_one, - sample_invite, - api_user_active, - mock_check_invite_token, - mock_dont_get_user_by_email, - mock_is_email_unique, - mock_register_user, - mock_send_verify_code, - mock_check_verify_code, - mock_get_user, - mock_update_user_attribute, - mock_add_user_to_service, - mock_accept_invite, - mock_get_service, - mock_get_service_templates, - mock_get_template_statistics, - mock_get_jobs, - mock_has_permissions, - mock_get_users_by_service, - mock_get_detailed_service, - mock_get_usage, - mocker, -): - mocker.patch('app.main.views.invites.check_token') - - # visit accept token page - response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) - 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' - } - - # get redirected to register from invite - response = client.post(url_for('main.register_from_invite'), data=data) - - # that sends user on to verify - response = client.post(url_for('main.verify'), data={'sms_code': '12345'}, follow_redirects=True) - - # when they post codes back to admin user should be added to - # service and sent on to dash board - expected_permissions = ['send_messages', 'manage_service', 'manage_api_keys'] - - with client.session_transaction() as session: - new_user_id = session['user_id'] - mock_add_user_to_service.assert_called_with(data['service'], new_user_id, expected_permissions) - mock_accept_invite.assert_called_with(data['service'], sample_invite['id']) - mock_check_verify_code.assert_called_once_with(new_user_id, '12345', 'sms') - assert service_one['id'] == session['service_id'] - - raw_html = response.data.decode('utf-8') - page = BeautifulSoup(raw_html, 'html.parser') - assert page.find('h1').text == 'Dashboard' - - def test_gives_message_if_token_has_expired( app_, client, diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index 9c6fd0e25..a366e5282 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -49,7 +49,8 @@ def test_register_creates_new_user_and_redirects_to_continue_page( user_data = {'name': 'Some One Valid', 'email_address': 'notfound@example.gov.uk', 'mobile_number': phone_number_to_register_with, - 'password': 'validPassword!' + 'password': 'validPassword!', + 'auth_type': 'sms_auth' } response = client.post(url_for('main.register'), data=user_data, follow_redirects=True) @@ -62,7 +63,8 @@ def test_register_creates_new_user_and_redirects_to_continue_page( mock_register_user.assert_called_with(user_data['name'], user_data['email_address'], user_data['mobile_number'], - user_data['password']) + user_data['password'], + user_data['auth_type']) def test_register_continue_handles_missing_session_sensibly( @@ -175,15 +177,21 @@ def test_register_from_invite_( "invited@user.com", ["manage_users"], "pending", - datetime.utcnow()) + datetime.utcnow(), + 'sms_auth') with client.session_transaction() as session: session['invited_user'] = invited_user.serialize() - response = client.post(url_for('main.register_from_invite'), - data={'name': 'Registered in another Browser', - 'email_address': invited_user.email_address, - 'mobile_number': '+4407700900460', - 'service': str(invited_user.id), - 'password': 'somreallyhardthingtoguess'}) + response = client.post( + url_for('main.register_from_invite'), + data={ + 'name': 'Registered in another Browser', + 'email_address': invited_user.email_address, + 'mobile_number': '+4407700900460', + 'service': str(invited_user.id), + 'password': 'somreallyhardthingtoguess', + 'auth_type': 'sms_auth' + } + ) assert response.status_code == 302 assert response.location == url_for('main.verify', _external=True) @@ -199,14 +207,20 @@ def test_register_from_invite_when_user_registers_in_another_browser( api_user_active.email_address, ["manage_users"], "pending", - datetime.utcnow()) + datetime.utcnow(), + 'sms_auth') with client.session_transaction() as session: session['invited_user'] = invited_user.serialize() - 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), - 'password': 'somreallyhardthingtoguess'}) + 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), + 'password': 'somreallyhardthingtoguess', + 'auth_type': 'sms_auth' + } + ) assert response.status_code == 302 assert response.location == url_for('main.verify', _external=True) diff --git a/tests/conftest.py b/tests/conftest.py index 2a1958781..925456a4e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1288,11 +1288,12 @@ def mock_send_change_email_verification(mocker): @pytest.fixture(scope='function') def mock_register_user(mocker, api_user_pending): - def _register(name, email_address, mobile_number, password): + def _register(name, email_address, mobile_number, password, auth_type): api_user_pending.name = name api_user_pending.email_address = email_address api_user_pending.mobile_number = mobile_number api_user_pending.password = password + api_user_pending.auth_type = auth_type return api_user_pending return mocker.patch('app.user_api_client.register_user', side_effect=_register) @@ -1910,7 +1911,9 @@ def sample_invite(mocker, service_one, status='pending'): service_id = service_one['id'] permissions = 'send_messages,manage_service,manage_api_keys' created_at = str(datetime.utcnow()) - return invite_json(id_, from_user, service_id, email_address, permissions, created_at, status) + auth_type = 'sms_auth' + + return invite_json(id_, from_user, service_id, email_address, permissions, created_at, status, auth_type) @pytest.fixture(scope='function') From 4c395628219a30350a0571a66c2bb52b05432c26 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 14 Nov 2017 13:30:31 +0000 Subject: [PATCH 3/7] make big old test give some clues about where it fails by checking response codes of requests --- tests/app/main/views/test_accept_invite.py | 66 ++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 6c6ec4abc..842466493 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -314,6 +314,72 @@ def test_signed_in_existing_user_cannot_use_anothers_invite( assert mock_accept_invite.call_count == 0 +def test_new_invited_user_verifies_and_added_to_service( + client, + service_one, + sample_invite, + api_user_active, + mock_check_invite_token, + mock_dont_get_user_by_email, + mock_is_email_unique, + mock_register_user, + mock_send_verify_code, + mock_check_verify_code, + mock_get_user, + mock_update_user_attribute, + mock_add_user_to_service, + mock_accept_invite, + mock_get_service, + mock_get_service_templates, + mock_get_template_statistics, + mock_get_jobs, + mock_has_permissions, + mock_get_users_by_service, + mock_get_detailed_service, + mock_get_usage, + mocker, +): + mocker.patch('app.main.views.invites.check_token') + + # visit accept token page + response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) + assert response.status_code == 302 + assert response.location == url_for('main.register_from_invite', _external=True) + + # get redirected to register from invite + 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', + # 'auth_type': 'sms_auth' + } + response = client.post(url_for('main.register_from_invite'), data=data) + assert response.status_code == 302 + assert response.location == url_for('main.verify', _external=True) + + # that sends user on to verify + response = client.post(url_for('main.verify'), data={'sms_code': '12345'}, follow_redirects=True) + assert response.status_code == 200 + + # when they post codes back to admin user should be added to + # service and sent on to dash board + expected_permissions = ['send_messages', 'manage_service', 'manage_api_keys'] + + with client.session_transaction() as session: + new_user_id = session['user_id'] + mock_add_user_to_service.assert_called_with(data['service'], new_user_id, expected_permissions) + mock_accept_invite.assert_called_with(data['service'], sample_invite['id']) + mock_check_verify_code.assert_called_once_with(new_user_id, '12345', 'sms') + assert service_one['id'] == session['service_id'] + + raw_html = response.data.decode('utf-8') + page = BeautifulSoup(raw_html, 'html.parser') + assert page.find('h1').text == 'Dashboard' + + def test_gives_message_if_token_has_expired( app_, client, From c8dbd819efbe814249354857b3933d7dd4c2f850 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 14 Nov 2017 15:03:05 +0000 Subject: [PATCH 4/7] add tests for registering from an email_auth invite --- app/main/views/register.py | 7 +- tests/app/main/views/test_register.py | 105 +++++++++++++++++++++++++- 2 files changed, 108 insertions(+), 4 deletions(-) diff --git a/app/main/views/register.py b/app/main/views/register.py index 9e92fa444..f94895695 100644 --- a/app/main/views/register.py +++ b/app/main/views/register.py @@ -43,10 +43,11 @@ def register(): @main.route('/register-from-invite', methods=['GET', 'POST']) def register_from_invite(): invited_user = session.get('invited_user') - form = RegisterUserFromInviteForm() if not invited_user: abort(404) + form = RegisterUserFromInviteForm() + if form.validate_on_submit(): if form.service.data != invited_user['service'] or form.email_address.data != invited_user['email_address']: abort(400) @@ -65,11 +66,11 @@ def register_from_invite(): return render_template('views/register-from-invite.html', email_address=invited_user['email_address'], form=form) -def _do_registration(form, service=None, send_sms=True, send_email=True): +def _do_registration(form, send_sms=True, send_email=True): if user_api_client.is_email_unique(form.email_address.data): user = user_api_client.register_user(form.name.data, form.email_address.data, - form.mobile_number.data, + form.mobile_number.data or None, form.password.data, form.auth_type.data) diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index a366e5282..544e69d8c 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -8,6 +8,7 @@ from flask import ( url_for, session ) +from flask_login import current_user from app.notify_client.models import InvitedUser @@ -165,7 +166,7 @@ def test_register_with_existing_email_sends_emails( assert response.location == url_for('main.registration_continue', _external=True) -def test_register_from_invite_( +def test_register_from_invite( client, fake_uuid, mock_is_email_unique, @@ -224,3 +225,105 @@ def test_register_from_invite_when_user_registers_in_another_browser( ) assert response.status_code == 302 assert response.location == url_for('main.verify', _external=True) + + +def test_register_from_email_auth_invite( + client, + sample_invite, + mock_is_email_unique, + mock_register_user, + mock_get_user, + mock_send_verify_email, + mock_send_verify_code, + mock_accept_invite, +): + sample_invite['auth_type'] = 'email_auth' + with client.session_transaction() as session: + session['invited_user'] = sample_invite + assert not current_user.is_authenticated + + data = { + 'name': 'invited user', + 'email_address': sample_invite['email_address'], + 'mobile_number': '07700900001', + 'password': 'FSLKAJHFNvdzxgfyst', + 'service': sample_invite['service'], + 'auth_type': 'email_auth', + } + + resp = client.post(url_for('main.register_from_invite'), data=data) + assert resp.status_code == 302 + assert resp.location == url_for('main.add_service', first='first', _external=True) + + # doesn't send any 2fa code + assert not mock_send_verify_email.called + assert not mock_send_verify_code.called + # creates user with email_auth set + mock_register_user.assert_called_once_with( + data['name'], + data['email_address'], + data['mobile_number'], + data['password'], + data['auth_type'] + ) + mock_accept_invite.assert_called_once_with(sample_invite['service'], sample_invite['id']) + # just logs them in + assert current_user.is_authenticated + + 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 + + +def test_can_register_email_auth_without_phone_number( + client, + sample_invite, + mock_is_email_unique, + mock_register_user, + mock_get_user, + mock_send_verify_email, + mock_send_verify_code, + mock_accept_invite, +): + sample_invite['auth_type'] = 'email_auth' + with client.session_transaction() as session: + session['invited_user'] = sample_invite + + data = { + 'name': 'invited user', + 'email_address': sample_invite['email_address'], + 'mobile_number': '', + 'password': 'FSLKAJHFNvdzxgfyst', + 'service': sample_invite['service'], + 'auth_type': 'email_auth' + } + + resp = client.post(url_for('main.register_from_invite'), data=data) + assert resp.status_code == 302 + assert resp.location == url_for('main.add_service', first='first', _external=True) + + mock_register_user.assert_called_once_with( + ANY, + ANY, + None, # mobile_number + ANY, + ANY + ) + + +def test_cannot_register_with_sms_auth_and_missing_mobile_number( + client, + mock_send_verify_code, + mock_get_user_by_email_not_found, + mock_login, +): + response = client.post(url_for('main.register'), + data={'name': 'Missing Mobile', + 'email_address': 'missing_mobile@example.gov.uk', + 'password': 'validPassword!'}) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + err = page.select_one('.error-message') + assert err.text.strip() == 'Can’t be empty' + assert err.attrs['data-error-label'] == 'mobile_number' From 8c14113da5ae8bbaec3fcf532a160930c38f7c82 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 14 Nov 2017 15:53:38 +0000 Subject: [PATCH 5/7] only show mobile number on register from invite page if user is sms_auth also clean up the way the form is invoked - it now populates from an invited_user object --- app/main/forms.py | 7 +++++++ app/main/views/register.py | 13 ++++++------- app/templates/views/register-from-invite.html | 11 +++++++---- tests/app/main/views/test_accept_invite.py | 2 +- tests/app/main/views/test_register.py | 16 ++++++++++++++++ 5 files changed, 37 insertions(+), 12 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 2363b7e28..f9e1e47c3 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -176,6 +176,13 @@ class RegisterUserForm(Form): class RegisterUserFromInviteForm(Form): + def __init__(self, invited_user): + super().__init__( + service=invited_user['service'], + email_address=invited_user['email_address'], + auth_type=invited_user['auth_type'], + ) + name = StringField( 'Full name', validators=[DataRequired(message='Can’t be empty')] diff --git a/app/main/views/register.py b/app/main/views/register.py index f94895695..0bdcf3ca4 100644 --- a/app/main/views/register.py +++ b/app/main/views/register.py @@ -46,24 +46,23 @@ def register_from_invite(): if not invited_user: abort(404) - form = RegisterUserFromInviteForm() + is_sms_auth = invited_user['auth_type'] == 'sms_auth' + + form = RegisterUserFromInviteForm(invited_user) if form.validate_on_submit(): if form.service.data != invited_user['service'] or form.email_address.data != invited_user['email_address']: abort(400) - _do_registration(form, send_email=False, send_sms=invited_user['auth_type'] == 'sms_auth') + _do_registration(form, send_email=False, send_sms=is_sms_auth) invite_api_client.accept_invite(invited_user['service'], invited_user['id']) - if invited_user['auth_type'] == 'sms_auth': + if is_sms_auth: return redirect(url_for('main.verify')) else: # we've already proven this user has email because they clicked the invite link, # so just activate them straight away return activate_user(session['user_details']['id']) - form.service.data = invited_user['service'] - form.email_address.data = invited_user['email_address'] - - return render_template('views/register-from-invite.html', email_address=invited_user['email_address'], form=form) + return render_template('views/register-from-invite.html', invited_user=invited_user, form=form) def _do_registration(form, send_sms=True, send_email=True): diff --git a/app/templates/views/register-from-invite.html b/app/templates/views/register-from-invite.html index 29f9572b8..4051b8086 100644 --- a/app/templates/views/register-from-invite.html +++ b/app/templates/views/register-from-invite.html @@ -11,16 +11,19 @@ Create an account

Create an account

-

Your account will be created with this email: {{email_address}}

+

Your account will be created with this email: {{invited_user.email_address}}

{{ textbox(form.name, width='3-4') }} -
- {{ textbox(form.mobile_number, width='3-4', hint='We’ll send you a security code by text message') }} -
+ {% if invited_user.auth_type == 'sms_auth' %} +
+ {{ textbox(form.mobile_number, width='3-4', hint='We’ll send you a security code by text message') }} +
+ {% endif %} {{ textbox(form.password, hint="At least 8 characters", width='3-4') }} {{ page_footer("Continue") }} {{form.service}} {{form.email_address}} + {{form.auth_type}}
diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 842466493..aa2f0de30 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -354,7 +354,7 @@ def test_new_invited_user_verifies_and_added_to_service( 'password': 'longpassword', 'mobile_number': '+447890123456', 'name': 'Invited User', - # 'auth_type': 'sms_auth' + 'auth_type': 'sms_auth' } response = client.post(url_for('main.register_from_invite'), data=data) assert response.status_code == 302 diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index 544e69d8c..ca83de216 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -327,3 +327,19 @@ def test_cannot_register_with_sms_auth_and_missing_mobile_number( err = page.select_one('.error-message') assert err.text.strip() == 'Can’t be empty' assert err.attrs['data-error-label'] == 'mobile_number' + + +def test_register_from_invite_form_doesnt_show_mobile_number_field_if_email_auth( + client, + sample_invite +): + sample_invite['auth_type'] = 'email_auth' + with client.session_transaction() as session: + session['invited_user'] = sample_invite + + response = client.get(url_for('main.register_from_invite')) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.find('input', attrs={'name': 'auth_type'}).attrs['value'] == 'email_auth' + assert page.find('input', attrs={'name': 'mobile_number'}) is None From 6df775fb11e5322de67343e74fc203d7afcceda9 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 14 Nov 2017 16:54:53 +0000 Subject: [PATCH 6/7] use mapping_table on user-profile page --- app/templates/views/user-profile.html | 48 ++++++++++++++++----------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/app/templates/views/user-profile.html b/app/templates/views/user-profile.html index 3ddd1ffc1..588d4d319 100644 --- a/app/templates/views/user-profile.html +++ b/app/templates/views/user-profile.html @@ -1,5 +1,6 @@ {% extends "withoutnav_template.html" %} {% from "components/table.html" import list_table, row, field %} +{% from "components/table.html" import mapping_table, row, text_field, optional_text_field, edit_field, field, boolean_field %} {% block per_page_title %} Your profile @@ -9,33 +10,40 @@

Your profile

- {% call(item, row_number) list_table( - [ - {'label': 'Name', 'value': current_user.name, 'url': url_for('.user_profile_name')}, - {'label': 'Email address', 'value': current_user.email_address, 'url': url_for('.user_profile_email')}, - {'label': 'Mobile number', 'value': current_user.mobile_number, 'url': url_for('.user_profile_mobile_number')}, - {'label': 'Password', 'value': 'Last changed ' + current_user.password_changed_at|format_delta, 'url': url_for('.user_profile_password')}, - ], - caption='Account settings', - field_headings=['Setting', 'Value', 'Link to change'], + {% call mapping_table( + caption='Your profile', + field_headings=['Label', 'Value', 'Action'], field_headings_visible=False, caption_visible=False ) %} - {% call field() %} - {{ item.label }} + {% call row() %} + {{ text_field('Name') }} + {{ text_field(current_user.name) }} + {{ edit_field('Change', url_for('.user_profile_name')) }} {% endcall %} - {% call field() %} - {{ item.value }} - {% endcall %} - {% call field(align='right') %} - {% if item.label == 'Email address' %} - {% if can_see_edit %} - Change - {% endif %} + + {% call row() %} + {{ text_field('Email address') }} + {{ text_field(current_user.email_address) }} + {% if can_see_edit %} + {{ edit_field('Change', url_for('.user_profile_email')) }} {% else %} - Change + {{ text_field('') }} {% endif %} {% endcall %} + + {% call row() %} + {{ text_field('Mobile number') }} + {{ optional_text_field(current_user.mobile_number) }} + {{ edit_field('Change', url_for('.user_profile_mobile_number')) }} + {% endcall %} + + {% call row() %} + {{ text_field('Password') }} + {{ text_field('Last changed ' + current_user.password_changed_at|format_delta) }} + {{ edit_field('Change', url_for('.user_profile_password')) }} + {% endcall %} + {% endcall %} {% endblock %} From 5353a26bbf3f483fbfcc807403169be5ec0841da Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 14 Nov 2017 17:01:04 +0000 Subject: [PATCH 7/7] make sure auth type is set when registering --- app/templates/views/register.html | 1 + tests/app/main/views/test_register.py | 2 ++ 2 files changed, 3 insertions(+) diff --git a/app/templates/views/register.html b/app/templates/views/register.html index bc7130c38..43d07bb14 100644 --- a/app/templates/views/register.html +++ b/app/templates/views/register.html @@ -19,6 +19,7 @@ Create an account {{ textbox(form.password, hint="At least 8 characters", width='3-4') }} + {{form.auth_type}} {{ page_footer("Continue") }} diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index ca83de216..ba5cc3b9e 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -16,6 +16,8 @@ def test_render_register_returns_template_with_form(client): response = client.get('/register') assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.find('input', attrs={'name': 'auth_type'}).attrs['value'] == 'sms_auth' assert 'Create an account' in response.get_data(as_text=True)