diff --git a/app/main/forms.py b/app/main/forms.py index 5c3c389ca..f9e1e47c3 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)) @@ -170,15 +171,31 @@ 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): - name = StringField('Full name', - validators=[DataRequired(message='Can’t be empty')]) - mobile_number = international_phone_number() + 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')] + ) + mobile_number = InternationalPhoneNumber('Mobile number', validators=[]) 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.data == 'sms_auth' and not field.data: + raise ValidationError('Can’t be empty') class PermissionsForm(Form): diff --git a/app/main/views/register.py b/app/main/views/register.py index 5e7f9d062..0bdcf3ca4 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,30 +42,36 @@ def register(): @main.route('/register-from-invite', methods=['GET', 'POST']) def register_from_invite(): - form = RegisterUserFromInviteForm() invited_user = session.get('invited_user') if not invited_user: abort(404) + 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) + _do_registration(form, send_email=False, send_sms=is_sms_auth) invite_api_client.accept_invite(invited_user['service'], invited_user['id']) - return redirect(url_for('main.verify')) + 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, 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.password.data) + form.mobile_number.data or None, + 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/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')) 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/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/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/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 %} 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..aa2f0de30 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 @@ -341,19 +343,26 @@ def test_new_invited_user_verifies_and_added_to_service( # 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' - } + 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 diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index 9c6fd0e25..ba5cc3b9e 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 @@ -15,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) @@ -49,7 +52,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 +66,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( @@ -163,7 +168,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, @@ -175,15 +180,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 +210,138 @@ 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) + + +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' + + +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 diff --git a/tests/conftest.py b/tests/conftest.py index 860633847..e1277a0d3 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')