diff --git a/app/main/forms.py b/app/main/forms.py index f4c2f6a39..650f72891 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -21,7 +21,7 @@ from wtforms import ( from wtforms.fields.html5 import EmailField, TelField from wtforms.validators import (DataRequired, Email, Length, Regexp, Optional) -from app.main.validators import (Blacklist, CsvFileValidator, ValidEmailDomainRegex, NoCommasInPlaceHolders) +from app.main.validators import (Blacklist, CsvFileValidator, ValidGovEmail, NoCommasInPlaceHolders) def get_time_value_and_label(future_time): @@ -48,12 +48,16 @@ def get_next_hours_from(now, hours=23): ] -def email_address(label='Email address'): - return EmailField(label, validators=[ +def email_address(label='Email address', gov_user=True): + validators = [ Length(min=5, max=255), DataRequired(message='Can’t be empty'), - Email(message='Enter a valid email address'), - ValidEmailDomainRegex()]) + Email(message='Enter a valid email address') + ] + + if gov_user: + validators.append(ValidGovEmail()) + return EmailField(label, validators) class UKMobileNumber(TelField): @@ -126,7 +130,7 @@ class PermissionsForm(Form): class InviteUserForm(PermissionsForm): - email_address = email_address('Email address') + email_address = email_address(gov_user=False) def __init__(self, invalid_email_address, *args, **kwargs): super(InviteUserForm, self).__init__(*args, **kwargs) @@ -242,7 +246,7 @@ class EmailTemplateForm(SMSTemplateForm): class ForgotPasswordForm(Form): - email_address = email_address() + email_address = email_address(gov_user=False) class NewPasswordForm(Form): diff --git a/app/main/validators.py b/app/main/validators.py index 6b92f64a2..c44b9774d 100644 --- a/app/main/validators.py +++ b/app/main/validators.py @@ -1,7 +1,9 @@ -import re from wtforms import ValidationError from notifications_utils.template import Template -from app.utils import Spreadsheet +from app.utils import ( + Spreadsheet, + is_gov_user +) from ._blacklisted_passwords import blacklisted_passwords @@ -26,17 +28,15 @@ class CsvFileValidator(object): raise ValidationError("{} isn’t a spreadsheet that Notify can read".format(field.data.filename)) -class ValidEmailDomainRegex(object): +class ValidGovEmail(object): def __call__(self, form, field): - from flask import (current_app, url_for) + from flask import url_for message = ( 'Enter a central government email address.' ' If you think you should have access' ' contact us').format(url_for('main.feedback')) - valid_domains = current_app.config.get('EMAIL_DOMAIN_REGEXES', []) - email_regex = "[^\@^\s]+@([^@^\\.^\\s]+\.)*({})$".format("|".join(valid_domains)) - if not re.match(email_regex, field.data.lower()): + if not is_gov_user(field.data.lower()): raise ValidationError(message) diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index 7fa36a05d..3f8a5e064 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -3,9 +3,15 @@ from flask import ( redirect, session, url_for, - current_app) + current_app +) -from flask_login import login_required +from flask_login import ( + current_user, + login_required +) + +from werkzeug.exceptions import abort from app.main import main from app.main.forms import AddServiceForm @@ -16,7 +22,32 @@ from app import ( user_api_client, service_api_client ) -from app.utils import email_safe + +from app.utils import ( + email_safe, + is_gov_user +) + + +def _add_invited_user_to_service(invited_user): + invitation = InvitedUser(**invited_user) + # if invited user add to service and redirect to dashboard + user = user_api_client.get_user(session['user_id']) + service_id = invited_user['service'] + user_api_client.add_user_to_service(service_id, user.id, invitation.permissions) + invite_api_client.accept_invite(service_id, invitation.id) + return service_id + + +def _create_service(service_name, email_from): + service_id = service_api_client.create_service(service_name=service_name, + active=False, + message_limit=current_app.config['DEFAULT_SERVICE_LIMIT'], + restricted=True, + user_id=session['user_id'], + email_from=email_from) + session['service_id'] = service_id + return service_id @main.route("/add-service", methods=['GET', 'POST']) @@ -24,25 +55,19 @@ from app.utils import email_safe def add_service(): invited_user = session.get('invited_user') if invited_user: - invitation = InvitedUser(**invited_user) - # if invited user add to service and redirect to dashboard - user = user_api_client.get_user(session['user_id']) - service_id = invited_user['service'] - user_api_client.add_user_to_service(service_id, user.id, invitation.permissions) - invite_api_client.accept_invite(service_id, invitation.id) + service_id = _add_invited_user_to_service(invited_user) return redirect(url_for('main.service_dashboard', service_id=service_id)) + if not is_gov_user(current_user.email_address): + abort(403) + form = AddServiceForm(service_api_client.find_all_service_email_from) heading = 'Which service do you want to set up notifications for?' + if form.validate_on_submit(): email_from = email_safe(form.name.data) - service_id = service_api_client.create_service(service_name=form.name.data, - active=False, - message_limit=current_app.config['DEFAULT_SERVICE_LIMIT'], - restricted=True, - user_id=session['user_id'], - email_from=email_from) - session['service_id'] = service_id + service_name = form.name.data + service_id = _create_service(service_name, email_from) if (len(service_api_client.get_services({'user_id': session['user_id']}).get('data', [])) > 1): return redirect(url_for('main.service_dashboard', service_id=service_id)) diff --git a/app/main/views/choose_service.py b/app/main/views/choose_service.py index 4b5f27f47..d5cd2e6b9 100644 --- a/app/main/views/choose_service.py +++ b/app/main/views/choose_service.py @@ -3,6 +3,7 @@ from flask_login import login_required, current_user from app.main import main from app import service_api_client from app.notify_client.service_api_client import ServicesBrowsableItem +from app.utils import is_gov_user @main.route("/services") @@ -11,7 +12,8 @@ def choose_service(): return render_template( 'views/choose-service.html', services=[ServicesBrowsableItem(x) for x in - service_api_client.get_services({'user_id': current_user.id})['data']] + service_api_client.get_services({'user_id': current_user.id})['data']], + can_add_service=is_gov_user(current_user.email_address) ) diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index 84c13bd81..cc71fc758 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -1,6 +1,7 @@ import json from flask import ( + abort, render_template, redirect, url_for, @@ -21,6 +22,8 @@ from app.main.forms import ( ConfirmPasswordForm ) +from app.utils import is_gov_user + from app import user_api_client NEW_EMAIL = 'new-email' @@ -31,7 +34,10 @@ NEW_MOBILE_PASSWORD_CONFIRMED = 'new-mob-password-confirmed' @main.route("/user-profile") @login_required def user_profile(): - return render_template('views/user-profile.html') + return render_template( + 'views/user-profile.html', + can_see_edit=is_gov_user(current_user.email_address) + ) @main.route("/user-profile/name", methods=['GET', 'POST']) @@ -56,6 +62,9 @@ def user_profile_name(): @login_required def user_profile_email(): + if not is_gov_user(current_user.email_address): + abort(403) + def _is_email_unique(email): return user_api_client.is_email_unique(email) form = ChangeEmailForm(_is_email_unique, diff --git a/app/templates/views/choose-service.html b/app/templates/views/choose-service.html index 5087d2a3c..fbc55ba4d 100644 --- a/app/templates/views/choose-service.html +++ b/app/templates/views/choose-service.html @@ -12,12 +12,14 @@ {{ browse_list(services) }} - {{ browse_list([ - { - 'title': 'Add a new service…', - 'link': url_for('.add_service') - }, - ]) }} + {% if can_add_service %} + {{ browse_list([ + { + 'title': 'Add a new service…', + 'link': url_for('.add_service') + }, + ]) }} + {% endif %} {% endblock %} diff --git a/app/templates/views/invite-user.html b/app/templates/views/invite-user.html index 83afb4214..ee392d935 100644 --- a/app/templates/views/invite-user.html +++ b/app/templates/views/invite-user.html @@ -16,7 +16,7 @@ Manage users – GOV.UK Notify
- {{ textbox(form.email_address, hint='Must be from a central government organisation', width='1-1', safe_error_message=True) }} + {{ textbox(form.email_address, width='1-1', safe_error_message=True) }} {% include 'views/manage-users/permissions.html' %} diff --git a/app/templates/views/user-profile.html b/app/templates/views/user-profile.html index 5b5c436af..d1e3da880 100644 --- a/app/templates/views/user-profile.html +++ b/app/templates/views/user-profile.html @@ -28,7 +28,13 @@ {{ item.value }} {% endcall %} {% call field(align='right') %} - Change + {% if item.label == 'Email address' %} + {% if can_see_edit %} + Change + {% endif %} + {% else %} + Change + {% endif %} {% endcall %} {% endcall %} diff --git a/app/utils.py b/app/utils.py index 78dec801f..7610f9ec8 100644 --- a/app/utils.py +++ b/app/utils.py @@ -3,7 +3,8 @@ import csv from io import StringIO from os import path from functools import wraps -from flask import (abort, session, request, redirect, url_for) +from flask import (abort, current_app, session, request, redirect, url_for) +from flask_login import current_user import pyexcel import pyexcel.ext.io import pyexcel.ext.xls @@ -41,7 +42,6 @@ def user_has_permissions(*permissions, admin_override=False, any_=False): def wrap(func): @wraps(func) def wrap_func(*args, **kwargs): - from flask_login import current_user if current_user and current_user.is_authenticated: if current_user.has_permissions( permissions=permissions, @@ -198,3 +198,9 @@ class Spreadsheet(): def get_help_argument(): return request.args.get('help') if request.args.get('help') in ('1', '2', '3') else None + + +def is_gov_user(email_address): + valid_domains = current_app.config['EMAIL_DOMAIN_REGEXES'] + email_regex = (r"[\.|@]({})$".format("|".join(valid_domains))) + return bool(re.search(email_regex, email_address.lower())) diff --git a/config.py b/config.py index a1d8f2670..c230a8041 100644 --- a/config.py +++ b/config.py @@ -44,24 +44,25 @@ class Config(object): TEST_MESSAGE_FILENAME = 'Test message' EMAIL_DOMAIN_REGEXES = [ - "gov\.uk", - "mod\.uk", - "mil\.uk", - "ddc-mod\.org", - "slc\.co\.uk", - "gov\.scot", - "parliament\.uk", - "nhs\.uk", - "nhs\.net", - "police\.uk", - "kainos\.com", - "salesforce\.com", - "bitzesty\.com", - "dclgdatamart\.co\.uk", - "valtech\.co\.uk", - "cgi\.com", - "capita\.co\.uk", - "ucds.email"] + r"gov\.uk", + r"mod\.uk", + r"mil\.uk", + r"ddc-mod\.org", + r"slc\.co\.uk", + r"gov\.scot", + r"parliament\.uk", + r"nhs\.uk", + r"nhs\.net", + r"police\.uk", + r"kainos\.com", + r"salesforce\.com", + r"bitzesty\.com", + r"dclgdatamart\.co\.uk", + r"valtech\.co\.uk", + r"cgi\.com", + r"capita\.co\.uk", + r"ucds\.email" + ] class Development(Config): diff --git a/tests/app/main/test_validators.py b/tests/app/main/test_validators.py index dd69bf842..4e09bb61c 100644 --- a/tests/app/main/test_validators.py +++ b/tests/app/main/test_validators.py @@ -1,6 +1,6 @@ import pytest from app.main.forms import RegisterUserForm, ServiceSmsSender -from app.main.validators import ValidEmailDomainRegex, NoCommasInPlaceHolders +from app.main.validators import ValidGovEmail, NoCommasInPlaceHolders from wtforms import ValidationError from unittest.mock import Mock @@ -85,7 +85,7 @@ def _gen_mock_field(x): ]) def test_valid_list_of_white_list_email_domains(app_, email): with app_.test_request_context(): - email_domain_validators = ValidEmailDomainRegex() + email_domain_validators = ValidGovEmail() email_domain_validators(None, _gen_mock_field(email)) @@ -117,7 +117,7 @@ def test_valid_list_of_white_list_email_domains(app_, email): ]) def test_invalid_list_of_white_list_email_domains(app_, email): with app_.test_request_context(): - email_domain_validators = ValidEmailDomainRegex() + email_domain_validators = ValidGovEmail() with pytest.raises(ValidationError): email_domain_validators(None, _gen_mock_field(email)) diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index 564181830..f3d492cf9 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -1,6 +1,7 @@ from flask import url_for, session from unittest.mock import ANY import app +from app.utils import is_gov_user def test_get_should_render_add_service_template(app_, @@ -101,3 +102,23 @@ def test_should_return_form_errors_with_duplicate_service_name_regardless_of_cas assert 'This service name is already in use' in response.get_data(as_text=True) app.service_api_client.find_all_service_email_from.assert_called_once_with() assert not mock_create_service.called + + +def test_non_whitelist_user_cannot_access_create_service_page(client, + mock_login, + mock_get_non_govuser, + api_nongov_user_active): + client.login(api_nongov_user_active) + assert not is_gov_user(api_nongov_user_active.email_address) + response = client.get(url_for('main.add_service')) + assert response.status_code == 403 + + +def test_non_whitelist_user_cannot_create_service(client, + mock_login, + mock_get_non_govuser, + api_nongov_user_active): + client.login(api_nongov_user_active) + assert not is_gov_user(api_nongov_user_active.email_address) + response = client.post(url_for('main.add_service'), data={'name': 'SERVICE TWO'}) + assert response.status_code == 403 diff --git a/tests/app/main/views/test_all_services.py b/tests/app/main/views/test_all_services.py index 28756451d..d0931558b 100644 --- a/tests/app/main/views/test_all_services.py +++ b/tests/app/main/views/test_all_services.py @@ -30,6 +30,16 @@ def test_all_service_returns_403_when_not_a_platform_admin(app_, assert response.status_code == 403 +def test_non_gov_user_cannot_see_add_service_button(client, + mock_login, + mock_get_non_govuser, + api_nongov_user_active): + client.login(api_nongov_user_active) + response = client.get(url_for('main.choose_service')) + assert 'Add a new service' not in response.get_data(as_text=True) + assert response.status_code == 200 + + def _login_user(client, mocker, platform_admin_user, service_one): mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) client.login(platform_admin_user) diff --git a/tests/app/main/views/test_forgot_password.py b/tests/app/main/views/test_forgot_password.py index c10a1e711..bc63777b0 100644 --- a/tests/app/main/views/test_forgot_password.py +++ b/tests/app/main/views/test_forgot_password.py @@ -1,5 +1,8 @@ +import pytest + from flask import url_for, Response from notifications_python_client.errors import HTTPError +from tests.conftest import api_user_active as create_active_user import app @@ -12,19 +15,25 @@ def test_should_render_forgot_password(app_): in response.get_data(as_text=True) +@pytest.mark.parametrize('email_address', [ + 'test@user.gov.uk', + 'someuser@notonwhitelist.com' +]) def test_should_redirect_to_password_reset_sent_for_valid_email( app_, - api_user_active, + fake_uuid, + email_address, mocker): with app_.test_request_context(): + sample_user = create_active_user(fake_uuid, email_address=email_address) mocker.patch('app.user_api_client.send_reset_password_url', return_value=None) response = app_.test_client().post( url_for('.forgot_password'), - data={'email_address': api_user_active.email_address}) + data={'email_address': sample_user.email_address}) assert response.status_code == 200 assert 'Click the link in the email to reset your password.' \ in response.get_data(as_text=True) - app.user_api_client.send_reset_password_url.assert_called_once_with(api_user_active.email_address) + app.user_api_client.send_reset_password_url.assert_called_once_with(sample_user.email_address) def test_should_redirect_to_password_reset_sent_for_missing_email( diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index f9d7faa61..2db57d61d 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1,8 +1,10 @@ +import pytest from flask import url_for from bs4 import BeautifulSoup import app from app.notify_client.models import InvitedUser -from tests.conftest import service_one as service_1 +from app.utils import is_gov_user +from tests.conftest import service_one as create_sample_service def test_should_show_overview_page( @@ -11,7 +13,7 @@ def test_should_show_overview_page( mocker, mock_get_invites_for_service ): - service = service_1(active_user_with_permissions) + service = create_sample_service(active_user_with_permissions) with app_.test_request_context(): with app_.test_client() as client: client.login(active_user_with_permissions, mocker, service) @@ -28,7 +30,7 @@ def test_should_show_page_for_one_user( active_user_with_permissions, mocker ): - service = service_1(active_user_with_permissions) + service = create_sample_service(active_user_with_permissions) with app_.test_request_context(): with app_.test_client() as client: client.login(active_user_with_permissions, mocker, service) @@ -44,7 +46,7 @@ def test_edit_user_permissions( mock_get_invites_for_service, mock_set_user_permissions ): - service = service_1(active_user_with_permissions) + service = create_sample_service(active_user_with_permissions) with app_.test_request_context(): with app_.test_client() as client: client.login(active_user_with_permissions, mocker, service) @@ -83,7 +85,7 @@ def test_edit_some_user_permissions( mock_get_invites_for_service, mock_set_user_permissions ): - service = service_1(active_user_with_permissions) + service = create_sample_service(active_user_with_permissions) data = [InvitedUser(**sample_invite)] with app_.test_request_context(): with app_.test_client() as client: @@ -119,7 +121,7 @@ def test_should_show_page_for_inviting_user( active_user_with_permissions, mocker ): - service = service_1(active_user_with_permissions) + service = create_sample_service(active_user_with_permissions) with app_.test_request_context(): with app_.test_client() as client: client.login(active_user_with_permissions, mocker, service) @@ -129,20 +131,26 @@ def test_should_show_page_for_inviting_user( assert response.status_code == 200 +@pytest.mark.parametrize('email_address, gov_user', [ + ('test@example.gov.uk', True), + ('test@nonwhitelist.com', False) +]) def test_invite_user( app_, active_user_with_permissions, mocker, - sample_invite + sample_invite, + email_address, + gov_user ): - service = service_1(active_user_with_permissions) - email_address = 'test@example.gov.uk' + service = create_sample_service(active_user_with_permissions) sample_invite['email_address'] = 'test@example.gov.uk' data = [InvitedUser(**sample_invite)] with app_.test_request_context(): with app_.test_client() as client: client.login(active_user_with_permissions, mocker, service) + 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)) @@ -178,7 +186,7 @@ def test_cancel_invited_user_cancels_user_invitations(app_, mocker.patch('app.invite_api_client.cancel_invited_user') import uuid invited_user_id = uuid.uuid4() - service = service_1(active_user_with_permissions) + service = create_sample_service(active_user_with_permissions) client.login(active_user_with_permissions, mocker, service) response = client.get(url_for('main.cancel_invited_user', service_id=service['id'], invited_user_id=invited_user_id)) @@ -191,7 +199,7 @@ def test_manage_users_shows_invited_user(app_, mocker, active_user_with_permissions, sample_invite): - service = service_1(active_user_with_permissions) + service = create_sample_service(active_user_with_permissions) data = [InvitedUser(**sample_invite)] with app_.test_request_context(): with app_.test_client() as client: @@ -219,7 +227,7 @@ def test_manage_users_does_not_show_accepted_invite(app_, sample_invite['id'] = invited_user_id sample_invite['status'] = 'accepted' data = [InvitedUser(**sample_invite)] - service = service_1(active_user_with_permissions) + service = create_sample_service(active_user_with_permissions) with app_.test_request_context(): with app_.test_client() as client: client.login(active_user_with_permissions, mocker, service) @@ -242,7 +250,7 @@ def test_user_cant_invite_themselves( active_user_with_permissions, mock_create_invite ): - service = service_1(active_user_with_permissions) + service = create_sample_service(active_user_with_permissions) with app_.test_request_context(): with app_.test_client() as client: client.login(active_user_with_permissions, mocker, service) diff --git a/tests/app/main/views/test_user_profile.py b/tests/app/main/views/test_user_profile.py index cf0047108..c2711ae05 100644 --- a/tests/app/main/views/test_user_profile.py +++ b/tests/app/main/views/test_user_profile.py @@ -266,3 +266,23 @@ def test_should_redirect_after_password_change(app_, assert response.status_code == 302 assert response.location == url_for( 'main.user_profile', _external=True) + + +def test_non_gov_user_cannot_see_change_email_link(client, + api_nongov_user_active, + mock_login, + mock_get_non_govuser): + client.login(api_nongov_user_active) + response = client.get(url_for('main.user_profile')) + assert '' not in response.get_data(as_text=True) + assert 'Your profile' in response.get_data(as_text=True) + assert response.status_code == 200 + + +def test_non_gov_user_cannot_access_change_email_page(client, + api_nongov_user_active, + mock_login, + mock_get_non_govuser): + client.login(api_nongov_user_active) + response = client.get(url_for('main.user_profile_email')) + assert response.status_code == 403 diff --git a/tests/conftest.py b/tests/conftest.py index d257e5c14..5a110ff71 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -488,12 +488,30 @@ def platform_admin_user(fake_uuid): @pytest.fixture(scope='function') -def api_user_active(fake_uuid): +def api_user_active(fake_uuid, email_address='test@user.gov.uk'): from app.notify_client.user_api_client import User user_data = {'id': fake_uuid, 'name': 'Test User', 'password': 'somepassword', - 'email_address': 'test@user.gov.uk', + 'email_address': email_address, + 'mobile_number': '07700 900762', + 'state': 'active', + 'failed_login_count': 0, + 'permissions': {}, + 'platform_admin': False, + 'password_changed_at': str(datetime.utcnow()) + } + user = User(user_data) + return user + + +@pytest.fixture(scope='function') +def api_nongov_user_active(fake_uuid): + from app.notify_client.user_api_client import User + user_data = {'id': fake_uuid, + 'name': 'Test User', + 'password': 'somepassword', + 'email_address': 'someuser@notonwhitelist.com', 'mobile_number': '07700 900762', 'state': 'active', 'failed_login_count': 0, @@ -597,6 +615,19 @@ def mock_register_user(mocker, api_user_pending): return mocker.patch('app.user_api_client.register_user', side_effect=_register) +@pytest.fixture(scope='function') +def mock_get_non_govuser(mocker, user=None): + if user is None: + user = api_user_active(fake_uuid(), email_address='someuser@notonwhitelist.com') + + def _get_user(id_): + user.id = id_ + return user + + return mocker.patch( + 'app.user_api_client.get_user', side_effect=_get_user) + + @pytest.fixture(scope='function') def mock_get_user(mocker, user=None): if user is None: