diff --git a/app/assets/stylesheets/components/tick-cross.scss b/app/assets/stylesheets/components/tick-cross.scss index 4f80e08b3..4cf3f1151 100644 --- a/app/assets/stylesheets/components/tick-cross.scss +++ b/app/assets/stylesheets/components/tick-cross.scss @@ -63,6 +63,11 @@ right: -135px; } + &-hint { + color: #6F777B; + padding-top: 5px; + } + } } diff --git a/app/main/forms.py b/app/main/forms.py index 75d15fb16..d100b6f2d 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -186,6 +186,14 @@ class PermissionsForm(Form): manage_templates = BooleanField("Add and edit templates") manage_service = BooleanField("Modify this service and its team") manage_api_keys = BooleanField("Create and revoke API keys") + login_authentication = RadioField( + 'Sign in using', + choices=[ + ('sms_auth', 'Text message code'), + ('email_auth', 'Email link'), + ], + validators=[DataRequired()] + ) class InviteUserForm(PermissionsForm): diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 8eae948b3..40e1dc047 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -19,7 +19,7 @@ from app.main.forms import ( InviteUserForm, PermissionsForm ) -from app import (user_api_client, service_api_client, invite_api_client) +from app import (user_api_client, current_service, service_api_client, invite_api_client) from app.utils import user_has_permissions @@ -38,6 +38,7 @@ def manage_users(service_id): users = user_api_client.get_users_for_service(service_id=service_id) invited_users = [invite for invite in invite_api_client.get_invites_for_service(service_id=service_id) if invite.status != 'accepted'] + return render_template( 'views/manage-users.html', users=users, @@ -51,7 +52,13 @@ def manage_users(service_id): @user_has_permissions('manage_users', admin_override=True) def invite_user(service_id): - form = InviteUserForm(invalid_email_address=current_user.email_address) + form = InviteUserForm( + invalid_email_address=current_user.email_address + ) + + service_has_email_auth = 'email_auth' in current_service['permissions'] + if not service_has_email_auth: + form.login_authentication.data = 'sms_auth' if form.validate_on_submit(): email_address = form.email_address.data @@ -60,7 +67,8 @@ def invite_user(service_id): current_user.id, service_id, email_address, - permissions + permissions, + form.login_authentication.data ) flash('Invite sent to {}'.format(invited_user.email_address), 'default_with_tick') @@ -68,7 +76,8 @@ def invite_user(service_id): return render_template( 'views/invite-user.html', - form=form + form=form, + service_has_email_auth=service_has_email_auth ) @@ -76,26 +85,35 @@ def invite_user(service_id): @login_required @user_has_permissions('manage_users', admin_override=True) def edit_user_permissions(service_id, user_id): + service_has_email_auth = 'email_auth' in current_service['permissions'] # TODO we should probably using the service id here in the get user # call as well. eg. /user/?&service=service_id user = user_api_client.get_user(user_id) - # Need to make the email address read only, or a disabled field? - # Do it through the template or the form class? - form = PermissionsForm(**{ - role: user.has_permissions(permissions=permissions) for role, permissions in roles.items() - }) + user_has_no_mobile_number = user.mobile_number is None + + form = PermissionsForm( + **{role: user.has_permissions(permissions=permissions) for role, permissions in roles.items()}, + login_authentication=user.auth_type + ) if form.validate_on_submit(): user_api_client.set_user_permissions( user_id, service_id, permissions=set(get_permissions_from_form(form)), ) + if service_has_email_auth: + user_api_client.update_user_attribute( + user_id, + auth_type=form.login_authentication.data + ) return redirect(url_for('.manage_users', service_id=service_id)) return render_template( 'views/edit-user-permissions.html', user=user, - form=form + form=form, + service_has_email_auth=service_has_email_auth, + user_has_no_mobile_number=user_has_no_mobile_number ) diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index eae4fc2d6..352aaef77 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -12,12 +12,13 @@ class InviteApiClient(NotifyAdminAPIClient): self.service_id = app.config['ADMIN_CLIENT_USER_NAME'] self.api_key = app.config['ADMIN_CLIENT_SECRET'] - def create_invite(self, invite_from_id, service_id, email_address, permissions): + def create_invite(self, invite_from_id, service_id, email_address, permissions, auth_type): data = { 'service': str(service_id), 'email_address': email_address, 'from_user': invite_from_id, - 'permissions': permissions + 'permissions': permissions, + 'auth_type': auth_type } data = _attach_current_user(data) resp = self.post(url='/service/{}/invite'.format(service_id), data=data) diff --git a/app/notify_client/models.py b/app/notify_client/models.py index bedbbd051..fe9775a36 100644 --- a/app/notify_client/models.py +++ b/app/notify_client/models.py @@ -10,6 +10,7 @@ class User(UserMixin): self._mobile_number = fields.get('mobile_number') self._password_changed_at = fields.get('password_changed_at') self._permissions = fields.get('permissions') + self._auth_type = fields.get('auth_type') self._failed_login_count = fields.get('failed_login_count') self._state = fields.get('state') self.max_failed_login_count = max_failed_login_count @@ -108,6 +109,14 @@ class User(UserMixin): return set(self._permissions[service_id]) >= set(permissions) return False + @property + def auth_type(self): + return self._auth_type + + @auth_type.setter + def auth_type(self, auth_type): + self._auth_type = auth_type + @property def failed_login_count(self): return self._failed_login_count @@ -155,6 +164,7 @@ class InvitedUser(object): self.permissions = [] self.status = status self.created_at = created_at + self.auth_type = auth_type def has_permissions(self, permissions): return set(self.permissions) > set(permissions) @@ -164,10 +174,12 @@ class InvitedUser(object): self.service, self.from_user, self.email_address, + self.auth_type, self.status) == (other.id, other.service, other.from_user, other.email_address, + other.auth_type, other.status)) def serialize(self, permissions_as_string=False): @@ -176,7 +188,8 @@ class InvitedUser(object): 'from_user': self.from_user, 'email_address': self.email_address, 'status': self.status, - 'created_at': str(self.created_at) + 'created_at': str(self.created_at), + 'auth_type': self.auth_type } if permissions_as_string: data['permissions'] = ','.join(self.permissions) diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 887339fab..3e3170689 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -6,7 +6,8 @@ from app.notify_client.models import User ALLOWED_ATTRIBUTES = { 'name', 'email_address', - 'mobile_number' + 'mobile_number', + 'auth_type', } diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index 373c5b3a6..9874e045e 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -63,6 +63,15 @@ user.has_permissions(permissions=['manage_api_keys']), 'Access API keys' ) }} + {% if 'email_auth' in current_service['permissions'] %} +
+ {% if user.auth_type == 'sms_auth' %} + Signs in with a text message code + {% else %} + Signs in with an email link + {% endif %} +
+ {% endif %} {% if current_user.has_permissions(['manage_users'], admin_override=True) %} {% if current_user.id != user.id %} @@ -104,6 +113,15 @@ user.has_permissions(permissions=['manage_api_keys']), 'Access API keys' ) }} + {% if 'email_auth' in current_service['permissions'] %} +
+ {% if user.auth_type == 'sms_auth' %} + Signs in with a text message code + {% else %} + Signs in with an email link + {% endif %} +
+ {% endif %}
  • {% if user.status == 'pending' and current_user.has_permissions(['manage_users']) %} diff --git a/app/templates/views/manage-users/permissions.html b/app/templates/views/manage-users/permissions.html index 8427a1b2e..e23eef804 100644 --- a/app/templates/views/manage-users/permissions.html +++ b/app/templates/views/manage-users/permissions.html @@ -1,4 +1,5 @@ {% from "components/checkbox.html" import checkbox %} +{% from "components/radios.html" import radios %}
    @@ -20,3 +21,7 @@
  • who the other team members are
    • + +{% if service_has_email_auth %} + {{ radios(form.login_authentication, disable=['sms_auth' if user_has_no_mobile_number]) }} +{% endif %} \ No newline at end of file diff --git a/tests/__init__.py b/tests/__init__.py index b48e2e33e..27057efbe 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -53,14 +53,14 @@ def service_json( created_at=None, letter_contact_block=None, inbound_api=None, - permissions=['email', 'sms'], + permissions=None, organisation_type='central', free_sms_fragment_limit=250000, ): if users is None: users = [] if permissions is None: - permissions = [] + permissions = ['email', 'sms'] if inbound_api is None: inbound_api = [] return { diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 2766cd032..c54da8a70 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -10,6 +10,7 @@ from tests.conftest import ( SERVICE_ONE_ID, active_user_with_permissions, active_user_view_permissions, + active_user_no_mobile, active_user_manage_template_permission, ) @@ -55,6 +56,94 @@ def test_should_show_overview_page( app.user_api_client.get_users_for_service.assert_called_once_with(service_id=SERVICE_ONE_ID) +@pytest.mark.parametrize('endpoint, extra_args, service_has_email_auth, auth_options_hidden', [ + ( + 'main.edit_user_permissions', + {'user_id': 0}, + True, + False + ), + ( + 'main.edit_user_permissions', + {'user_id': 0}, + False, + True + ), + ( + 'main.invite_user', + {}, + True, + False + ), + ( + 'main.invite_user', + {}, + False, + True + ) +]) +def test_service_with_no_email_auth_hides_auth_type_options( + client_request, + endpoint, + extra_args, + service_has_email_auth, + auth_options_hidden, + service_one +): + if service_has_email_auth: + service_one['permissions'].append('email_auth') + page = client_request.get(endpoint, service_id=service_one['id'], **extra_args) + assert (page.find('input', attrs={"name": "login_authentication"}) is None) == auth_options_hidden + + +@pytest.mark.parametrize('service_has_email_auth, displays_auth_type', [ + (True, True), + (False, False) +]) +def test_manage_users_page_shows_member_auth_type_if_service_has_email_auth_activated( + client_request, + service_has_email_auth, + service_one, + mock_get_users_by_service, + mock_get_invites_for_service, + displays_auth_type +): + if service_has_email_auth: + service_one['permissions'].append('email_auth') + page = client_request.get('main.manage_users', service_id=service_one['id']) + assert bool(page.select_one('.tick-cross-list-hint')) == displays_auth_type + + +@pytest.mark.parametrize('user, sms_option_disabled', [ + ( + active_user_no_mobile, + True, + ), + ( + active_user_with_permissions, + False, + ), +]) +def test_user_with_no_mobile_number_cant_be_set_to_sms_auth( + client_request, + user, + sms_option_disabled, + service_one, + mocker +): + service_one['permissions'].append('email_auth') + test_user = mocker.patch('app.user_api_client.get_user', return_value=user(mocker)) + + page = client_request.get( + 'main.edit_user_permissions', + service_id=service_one['id'], + user_id=test_user.id + ) + + sms_auth_radio_button = page.select_one('input[value="sms_auth"]') + assert sms_auth_radio_button.has_attr("disabled") == sms_option_disabled + + @pytest.mark.parametrize('endpoint, extra_args, expected_checkboxes', [ ( 'main.edit_user_permissions', @@ -167,6 +256,60 @@ def test_edit_some_user_permissions( ) +@pytest.mark.parametrize('auth_type', ['email_auth', 'sms_auth']) +def test_edit_user_permissions_including_authentication_with_email_auth_service( + logged_in_client, + active_user_with_permissions, + mocker, + mock_get_invites_for_service, + mock_set_user_permissions, + mock_update_user_attribute, + service_one, + auth_type +): + service_one['permissions'].append('email_auth') + + response = logged_in_client.post( + url_for( + 'main.edit_user_permissions', + service_id=service_one['id'], + user_id=active_user_with_permissions.id + ), + data={ + 'email_address': active_user_with_permissions.email_address, + 'send_messages': 'y', + 'manage_templates': 'y', + 'manage_service': 'y', + 'manage_api_keys': 'y', + 'login_authentication': auth_type + } + ) + + mock_set_user_permissions.assert_called_with( + str(active_user_with_permissions.id), + service_one['id'], + permissions={ + 'send_texts', + 'send_emails', + 'send_letters', + 'manage_users', + 'manage_templates', + 'manage_settings', + 'manage_api_keys', + 'view_activity' + } + ) + mock_update_user_attribute.assert_called_with( + str(active_user_with_permissions.id), + auth_type=auth_type + ) + + assert response.status_code == 302 + assert response.location == url_for( + 'main.manage_users', service_id=service_one['id'], _external=True + ) + + def test_should_show_page_for_inviting_user( logged_in_client, active_user_with_permissions, @@ -220,7 +363,60 @@ def test_invite_user( app.invite_api_client.create_invite.assert_called_once_with(sample_invite['from_user'], sample_invite['service'], email_address, - expected_permissions) + expected_permissions, + 'sms_auth') + + +@pytest.mark.parametrize('auth_type', [ + ('sms_auth'), + ('email_auth') +]) +@pytest.mark.parametrize('email_address, gov_user', [ + ('test@example.gov.uk', True), + ('test@nonwhitelist.com', False) +]) +def test_invite_user_with_email_auth_service( + logged_in_client, + active_user_with_permissions, + sample_invite, + email_address, + gov_user, + mocker, + service_one, + auth_type +): + service_one['permissions'].append('email_auth') + sample_invite['email_address'] = 'test@example.gov.uk' + + data = [InvitedUser(**sample_invite)] + assert is_gov_user(email_address) == gov_user + mocker.patch('app.invite_api_client.get_invites_for_service', return_value=data) + mocker.patch('app.user_api_client.get_users_for_service', return_value=[active_user_with_permissions]) + mocker.patch('app.invite_api_client.create_invite', return_value=InvitedUser(**sample_invite)) + response = logged_in_client.post( + url_for('main.invite_user', service_id=service_one['id']), + data={'email_address': email_address, + 'send_messages': 'y', + 'manage_templates': 'y', + 'manage_service': 'y', + 'manage_api_keys': 'y', + 'login_authentication': auth_type}, + follow_redirects=True + ) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'Team members' + flash_banner = page.find('div', class_='banner-default-with-tick').string.strip() + assert flash_banner == 'Invite sent to test@example.gov.uk' + + expected_permissions = 'manage_api_keys,manage_settings,manage_templates,manage_users,send_emails,send_letters,send_texts,view_activity' # noqa + + app.invite_api_client.create_invite.assert_called_once_with(sample_invite['from_user'], + sample_invite['service'], + email_address, + expected_permissions, + auth_type) def test_cancel_invited_user_cancels_user_invitations( diff --git a/tests/conftest.py b/tests/conftest.py index d3a2a1a7d..d9e7f6771 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1003,7 +1003,8 @@ def platform_admin_user(fake_uuid): 'manage_settings', 'manage_api_keys', 'view_activity']}, - 'platform_admin': True + 'platform_admin': True, + 'auth_type': 'sms_auth' } user = User(user_data) return user @@ -1021,6 +1022,7 @@ def api_user_active(fake_uuid, email_address='test@user.gov.uk'): 'failed_login_count': 0, 'permissions': {}, 'platform_admin': False, + 'auth_type': 'sms_auth', 'password_changed_at': str(datetime.utcnow()) } user = User(user_data) @@ -1039,6 +1041,7 @@ def api_nongov_user_active(fake_uuid): 'failed_login_count': 0, 'permissions': {}, 'platform_admin': False, + 'auth_type': 'sms_auth', 'password_changed_at': str(datetime.utcnow()) } user = User(user_data) @@ -1065,7 +1068,35 @@ def active_user_with_permissions(fake_uuid): 'manage_settings', 'manage_api_keys', 'view_activity']}, - 'platform_admin': False + 'platform_admin': False, + 'auth_type': 'sms_auth' + } + user = User(user_data) + return user + + +@pytest.fixture(scope='function') +def active_user_no_mobile(fake_uuid): + from app.notify_client.user_api_client import User + + user_data = {'id': fake_uuid, + 'name': 'Test User', + 'password': 'somepassword', + 'password_changed_at': str(datetime.utcnow()), + 'email_address': 'test@user.gov.uk', + 'mobile_number': None, + 'state': 'active', + 'failed_login_count': 0, + 'permissions': {SERVICE_ONE_ID: ['send_texts', + 'send_emails', + 'send_letters', + 'manage_users', + 'manage_templates', + 'manage_settings', + 'manage_api_keys', + 'view_activity']}, + 'platform_admin': False, + 'auth_type': 'email_auth' } user = User(user_data) return user @@ -1084,7 +1115,8 @@ def active_user_view_permissions(fake_uuid): 'state': 'active', 'failed_login_count': 0, 'permissions': {SERVICE_ONE_ID: ['view_activity']}, - 'platform_admin': False + 'platform_admin': False, + 'auth_type': 'sms_auth' } user = User(user_data) return user @@ -1107,7 +1139,8 @@ def active_user_manage_template_permission(fake_uuid): 'manage_templates', 'view_activity', ]}, - 'platform_admin': False + 'platform_admin': False, + 'auth_type': 'sms_auth' } user = User(user_data) return user @@ -1123,7 +1156,8 @@ def api_user_locked(fake_uuid): 'mobile_number': '07700 900762', 'state': 'active', 'failed_login_count': 5, - 'permissions': {} + 'permissions': {}, + 'auth_type': 'sms_auth' } user = User(user_data) return user @@ -1140,7 +1174,8 @@ def api_user_request_password_reset(fake_uuid): 'state': 'active', 'failed_login_count': 5, 'permissions': {}, - 'password_changed_at': None + 'password_changed_at': None, + 'auth_type': 'sms_auth' } user = User(user_data) return user @@ -1157,6 +1192,7 @@ def api_user_changed_password(fake_uuid): 'state': 'active', 'failed_login_count': 5, 'permissions': {}, + 'auth_type': 'sms_auth', 'password_changed_at': str(datetime.utcnow() + timedelta(minutes=1)) } user = User(user_data) @@ -1753,6 +1789,7 @@ def mock_get_users_by_service(mocker): 'password_changed_at': None, 'name': 'Test User', 'email_address': 'notify@digital.cabinet-office.gov.uk', + 'auth_type': 'sms_auth', 'failed_login_count': 0}] return [User(data[0])]