From 1b046ab94d23ed6349e35bde023bfd3b7917b5ed Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Tue, 25 Oct 2016 18:08:20 +0100
Subject: [PATCH 01/22] Update invite user form to allow non-whitelist users
 through

---
 app/main/forms.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/main/forms.py b/app/main/forms.py
index f4c2f6a39..8122236f2 100644
--- a/app/main/forms.py
+++ b/app/main/forms.py
@@ -126,7 +126,11 @@ class PermissionsForm(Form):
 
 
 class InviteUserForm(PermissionsForm):
-    email_address = email_address('Email address')
+    email_address = EmailField('Email address', validators=[
+        Length(min=5, max=255),
+        DataRequired(message='Can’t be empty'),
+        Email(message='Enter a valid email address')
+        ])
 
     def __init__(self, invalid_email_address, *args, **kwargs):
         super(InviteUserForm, self).__init__(*args, **kwargs)

From f33e0d0e94eb1826e7de1673feaacf4708a18f66 Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Tue, 25 Oct 2016 18:10:15 +0100
Subject: [PATCH 02/22] Add function to check if given email in whitelist

---
 app/utils.py | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/app/utils.py b/app/utils.py
index 72232cc6b..031ad7b1c 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,
@@ -201,3 +201,9 @@ class Spreadsheet():
 
 def get_help_argument():
     return request.args.get('help') if request.args.get('help') in ('1', '2', '3') else None
+
+
+def user_in_whitelist(email_address):
+    valid_domains = current_app.config.get('EMAIL_DOMAIN_REGEXES', [])
+    email_regex = "[^\@^\s]+@([^@^\\.^\\s]+\.)*({})$".format("|".join(valid_domains))
+    return bool(re.match(email_regex, email_address))

From 84762edef48164531249f421a9fa3b838d1e0d97 Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Tue, 25 Oct 2016 18:11:37 +0100
Subject: [PATCH 03/22] Abort 403 if nonwhitelist user tries to add service

---
 app/main/views/add_service.py            | 31 ++++++++++++++++++------
 tests/app/main/views/test_add_service.py |  8 ++++++
 2 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py
index 7fa36a05d..7cc588e54 100644
--- a/app/main/views/add_service.py
+++ b/app/main/views/add_service.py
@@ -1,11 +1,19 @@
+import re
+
 from flask import (
     render_template,
     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 +24,11 @@ from app import (
     user_api_client,
     service_api_client
 )
-from app.utils import email_safe
+
+from app.utils import (
+    email_safe,
+    user_in_whitelist
+)
 
 
 @main.route("/add-service", methods=['GET', 'POST'])
@@ -61,8 +73,11 @@ def add_service():
             help=1
         ))
     else:
-        return render_template(
-            'views/add-service.html',
-            form=form,
-            heading=heading
-        )
+        if not user_in_whitelist(current_user.email_address):
+            abort(403)
+        else:
+            return render_template(
+                'views/add-service.html',
+                form=form,
+                heading=heading
+            )
diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py
index 564181830..6743febf3 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 user_in_whitelist
 
 
 def test_get_should_render_add_service_template(app_,
@@ -101,3 +102,10 @@ 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_add_service(app_, nonwhitelist_user, mocker, client):
+    client.login(nonwhitelist_user, mocker)
+    assert not user_in_whitelist(nonwhitelist_user.email_address)
+    response = client.get(url_for('main.add_service'))
+    assert response.status_code == 403

From bb857822308a6dfc04d57ba5a63dbb46f2b8d162 Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Tue, 25 Oct 2016 18:12:46 +0100
Subject: [PATCH 04/22] Remove link for adding service if nonwhitelist user

---
 app/main/views/choose_service.py        |  8 ++++++--
 app/templates/views/choose-service.html |  2 ++
 tests/conftest.py                       | 16 ++++++++++++++++
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/app/main/views/choose_service.py b/app/main/views/choose_service.py
index 4b5f27f47..48bf81e39 100644
--- a/app/main/views/choose_service.py
+++ b/app/main/views/choose_service.py
@@ -1,8 +1,11 @@
-from flask import (render_template, redirect, url_for, session)
+import re
+
+from flask import (current_app, render_template, redirect, url_for, session)
 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 user_in_whitelist
 
 
 @main.route("/services")
@@ -11,7 +14,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=user_in_whitelist(current_user.email_address)
     )
 
 
diff --git a/app/templates/views/choose-service.html b/app/templates/views/choose-service.html
index 5087d2a3c..f55bda167 100644
--- a/app/templates/views/choose-service.html
+++ b/app/templates/views/choose-service.html
@@ -12,12 +12,14 @@
   </h1>
 
   {{ browse_list(services) }}
+  {% if can_add_service %}
   {{ browse_list([
     {
       'title': 'Add a new service…',
       'link': url_for('.add_service')
     },
   ]) }}
+  {% endif %}
 
 
 {% endblock %}
diff --git a/tests/conftest.py b/tests/conftest.py
index d257e5c14..b976771d1 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -487,6 +487,22 @@ def platform_admin_user(fake_uuid):
     return user
 
 
+@pytest.fixture(scope='function')
+def nonwhitelist_user(fake_uuid):
+    from app.notify_client.user_api_client import User
+    user_data = {'id': fake_uuid,
+                 'name': 'Platform admin user',
+                 'password': 'somepassword',
+                 'email_address': 'someuser@notonwhitelist.com',
+                 'mobile_number': '07700 900762',
+                 'state': 'active',
+                 'failed_login_count': 0,
+                 'permissions': {}
+                 }
+    user = User(user_data)
+    return user
+
+
 @pytest.fixture(scope='function')
 def api_user_active(fake_uuid):
     from app.notify_client.user_api_client import User

From 78aeb8934b4ee704cbb2d0009a86d9aaf498c6d3 Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Tue, 25 Oct 2016 18:13:50 +0100
Subject: [PATCH 05/22] Add test to invite nonwhitelist user and refactor

---
 config.py                                 |  3 ++-
 tests/app/main/views/test_manage_users.py | 12 ++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/config.py b/config.py
index a1d8f2670..837a27fae 100644
--- a/config.py
+++ b/config.py
@@ -61,7 +61,8 @@ class Config(object):
         "valtech\.co\.uk",
         "cgi\.com",
         "capita\.co\.uk",
-        "ucds.email"]
+        "ucds.email"
+    ]
 
 
 class Development(Config):
diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py
index f9d7faa61..6cde4873e 100644
--- a/tests/app/main/views/test_manage_users.py
+++ b/tests/app/main/views/test_manage_users.py
@@ -1,7 +1,9 @@
+import pytest
 from flask import url_for
 from bs4 import BeautifulSoup
 import app
 from app.notify_client.models import InvitedUser
+from app.utils import user_in_whitelist
 from tests.conftest import service_one as service_1
 
 
@@ -129,20 +131,26 @@ def test_should_show_page_for_inviting_user(
         assert response.status_code == 200
 
 
+@pytest.mark.parametrize('email_address, whitelist_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,
+        whitelist_user
 ):
     service = service_1(active_user_with_permissions)
-    email_address = 'test@example.gov.uk'
     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 user_in_whitelist(email_address) == whitelist_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))

From 1fb0e225705e2622c02b2491471f5848917607fe Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Wed, 26 Oct 2016 14:01:01 +0100
Subject: [PATCH 06/22] Update forgotten password to allow non-gov with test

---
 app/main/forms.py                            | 21 ++++++++++----------
 tests/app/main/views/test_forgot_password.py | 15 +++++++++++---
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/app/main/forms.py b/app/main/forms.py
index 8122236f2..76eab0310 100644
--- a/app/main/forms.py
+++ b/app/main/forms.py
@@ -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(ValidEmailDomainRegex())
+    return EmailField(label, validators)
 
 
 class UKMobileNumber(TelField):
@@ -126,11 +130,7 @@ class PermissionsForm(Form):
 
 
 class InviteUserForm(PermissionsForm):
-    email_address = EmailField('Email address', validators=[
-        Length(min=5, max=255),
-        DataRequired(message='Can’t be empty'),
-        Email(message='Enter a valid email address')
-        ])
+    email_address = email_address(gov_user=False)
 
     def __init__(self, invalid_email_address, *args, **kwargs):
         super(InviteUserForm, self).__init__(*args, **kwargs)
@@ -246,7 +246,8 @@ class EmailTemplateForm(SMSTemplateForm):
 
 
 class ForgotPasswordForm(Form):
-    email_address = email_address()
+    # email_address = email_address()
+    email_address = email_address(gov_user=False)
 
 
 class NewPasswordForm(Form):
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(

From da0c03e8a1ec9ec04b44123ab59be2d6776fa7fc Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Wed, 26 Oct 2016 14:02:23 +0100
Subject: [PATCH 07/22] Remove gov-only email text on page

---
 app/templates/views/invite-user.html | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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
   <div class="grid-row">
     <form method="post" class="column-three-quarters">
 
-      {{ 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' %}
 

From 4203c7c2509d77030690afd0e431c17e3ac4a1cf Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Wed, 26 Oct 2016 14:03:18 +0100
Subject: [PATCH 08/22] Refactor creating a non-gov user

---
 tests/app/main/views/test_add_service.py | 8 +++++---
 tests/conftest.py                        | 4 ++--
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py
index 6743febf3..3947ce013 100644
--- a/tests/app/main/views/test_add_service.py
+++ b/tests/app/main/views/test_add_service.py
@@ -2,6 +2,7 @@ from flask import url_for, session
 from unittest.mock import ANY
 import app
 from app.utils import user_in_whitelist
+from tests.conftest import api_user_active as create_active_user
 
 
 def test_get_should_render_add_service_template(app_,
@@ -104,8 +105,9 @@ def test_should_return_form_errors_with_duplicate_service_name_regardless_of_cas
             assert not mock_create_service.called
 
 
-def test_non_whitelist_user_cannot_add_service(app_, nonwhitelist_user, mocker, client):
-    client.login(nonwhitelist_user, mocker)
-    assert not user_in_whitelist(nonwhitelist_user.email_address)
+def test_non_whitelist_user_cannot_add_service(app_, mocker, client, fake_uuid):
+    non_whitelist_user = create_active_user(fake_uuid, 'someuser@notonwhitelist.com')
+    client.login(non_whitelist_user, mocker)
+    assert not user_in_whitelist(non_whitelist_user.email_address)
     response = client.get(url_for('main.add_service'))
     assert response.status_code == 403
diff --git a/tests/conftest.py b/tests/conftest.py
index b976771d1..32a8d7c30 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -504,12 +504,12 @@ def nonwhitelist_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,

From cd3a8bf533a9483fcb5cebfe9fb9d43546eaa4a8 Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Wed, 26 Oct 2016 14:15:55 +0100
Subject: [PATCH 09/22] Remove whitelist user fixture and refactor

---
 tests/app/main/views/test_manage_users.py | 22 +++++++++++-----------
 tests/conftest.py                         | 16 ----------------
 2 files changed, 11 insertions(+), 27 deletions(-)

diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py
index 6cde4873e..87e04944f 100644
--- a/tests/app/main/views/test_manage_users.py
+++ b/tests/app/main/views/test_manage_users.py
@@ -4,7 +4,7 @@ from bs4 import BeautifulSoup
 import app
 from app.notify_client.models import InvitedUser
 from app.utils import user_in_whitelist
-from tests.conftest import service_one as service_1
+from tests.conftest import service_one as create_sample_service
 
 
 def test_should_show_overview_page(
@@ -13,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)
@@ -30,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)
@@ -46,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)
@@ -85,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:
@@ -121,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)
@@ -143,7 +143,7 @@ def test_invite_user(
         email_address,
         whitelist_user
 ):
-    service = service_1(active_user_with_permissions)
+    service = create_sample_service(active_user_with_permissions)
     sample_invite['email_address'] = 'test@example.gov.uk'
 
     data = [InvitedUser(**sample_invite)]
@@ -186,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))
@@ -199,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:
@@ -227,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)
@@ -250,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/conftest.py b/tests/conftest.py
index 32a8d7c30..4e13cf044 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -487,22 +487,6 @@ def platform_admin_user(fake_uuid):
     return user
 
 
-@pytest.fixture(scope='function')
-def nonwhitelist_user(fake_uuid):
-    from app.notify_client.user_api_client import User
-    user_data = {'id': fake_uuid,
-                 'name': 'Platform admin user',
-                 'password': 'somepassword',
-                 'email_address': 'someuser@notonwhitelist.com',
-                 'mobile_number': '07700 900762',
-                 'state': 'active',
-                 'failed_login_count': 0,
-                 'permissions': {}
-                 }
-    user = User(user_data)
-    return user
-
-
 @pytest.fixture(scope='function')
 def api_user_active(fake_uuid, email_address='test@user.gov.uk'):
     from app.notify_client.user_api_client import User

From 26a985720cf5f6da29f6a69a634069805aee09a8 Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Wed, 26 Oct 2016 15:44:24 +0100
Subject: [PATCH 10/22] fix 500 errors with excel files > 500k size limit

werkzeug's internal workings keep files under 500kb in memory, and files
greater than 500kb as a TemporaryFile

(https://github.com/pallets/werkzeug/blob/0.11-maintenance/werkzeug/formparser.py#L38)

when we encounter a CSV or TSV, we call normalise_newlines, which invokes
`.read()`, however when we were passing straight into pyexcel, we called
`file.getvalue()` - this exists on BytesIO (small files) but not on
TemporaryFile objects (large files) - we were seeing 500 errors
---
 app/utils.py                      |  5 +----
 tests/app/main/views/test_send.py |  1 -
 tests/app/test_utils.py           | 13 +++++++++++--
 3 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/app/utils.py b/app/utils.py
index 72232cc6b..78dec801f 100644
--- a/app/utils.py
+++ b/app/utils.py
@@ -173,18 +173,15 @@ class Spreadsheet():
 
     @classmethod
     def from_rows(cls, rows, filename=''):
-
         with StringIO() as converted:
             output = csv.writer(converted)
 
             for row in rows:
                 output.writerow(row)
-
             return cls(converted.getvalue(), filename)
 
     @classmethod
     def from_file(cls, file_content, filename=''):
-
         extension = cls.get_extension(filename)
 
         if extension == 'csv':
@@ -195,7 +192,7 @@ class Spreadsheet():
 
         return cls.from_rows(pyexcel.get_sheet(
             file_type=extension,
-            file_content=file_content.getvalue()
+            file_content=file_content.read()
         ).to_array(), filename)
 
 
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index cecaac6dc..fbcd64280 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -8,7 +8,6 @@ from bs4 import BeautifulSoup
 from functools import partial
 from flask import url_for
 from tests import validate_route_permission
-from datetime import datetime
 
 template_types = ['email', 'sms']
 
diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py
index 14495ab2d..be62799e4 100644
--- a/tests/app/test_utils.py
+++ b/tests/app/test_utils.py
@@ -1,9 +1,12 @@
-import pytest
+from pathlib import Path
 from io import StringIO
-from app.utils import email_safe, generate_notifications_csv, generate_previous_dict, generate_next_dict
 from csv import DictReader
+
+import pytest
 from freezegun import freeze_time
 
+from app.utils import email_safe, generate_notifications_csv, generate_previous_dict, generate_next_dict, Spreadsheet
+
 
 def test_email_safe_return_dot_separated_email_domain():
     test_name = 'SOME service  with+stuff+ b123'
@@ -67,3 +70,9 @@ def test_generate_next_dict(client):
 def test_generate_previous_next_dict_adds_other_url_args(client):
     ret = generate_next_dict('main.view_notifications', 'foo', 2, {'message_type': 'blah'})
     assert 'notifications/blah' in ret['url']
+
+
+def test_can_create_spreadsheet_from_large_excel_file():
+    with open(str(Path.cwd() / 'tests' / 'spreadsheet_files' / 'excel 2007.xlsx'), 'rb') as xl:
+        ret = Spreadsheet.from_file(xl, filename='xl.xlsx')
+    assert ret.as_csv_data

From df313ad3d4c01ac6ece4f7cc4deca91e90b7633d Mon Sep 17 00:00:00 2001
From: bandesz <bandesz@gmail.com>
Date: Thu, 27 Oct 2016 13:13:49 +0100
Subject: [PATCH 11/22] Add minimal 404 page for Cloudfront

---
 app/assets/error_pages/404_cloudfront.html | 8 ++++++++
 1 file changed, 8 insertions(+)
 create mode 100644 app/assets/error_pages/404_cloudfront.html

diff --git a/app/assets/error_pages/404_cloudfront.html b/app/assets/error_pages/404_cloudfront.html
new file mode 100644
index 000000000..2af708ad6
--- /dev/null
+++ b/app/assets/error_pages/404_cloudfront.html
@@ -0,0 +1,8 @@
+<html>
+  <head>
+    <title>404 - Not found</title>
+  </head>
+  <body>
+    <h1>404 - Not found</h1>
+  </body>
+</html>

From 5ecdbb859682c5bdd26cebf90f1812e448b308d2 Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Fri, 28 Oct 2016 10:45:05 +0100
Subject: [PATCH 12/22] Refactor to use a cleaner and lean regex

---
 app/main/forms.py                 |  5 ++---
 app/main/validators.py            | 14 ++++++-------
 app/utils.py                      |  8 ++++----
 config.py                         | 34 +++++++++++++++----------------
 tests/app/main/test_validators.py |  6 +++---
 5 files changed, 33 insertions(+), 34 deletions(-)

diff --git a/app/main/forms.py b/app/main/forms.py
index 76eab0310..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):
@@ -56,7 +56,7 @@ def email_address(label='Email address', gov_user=True):
     ]
 
     if gov_user:
-        validators.append(ValidEmailDomainRegex())
+        validators.append(ValidGovEmail())
     return EmailField(label, validators)
 
 
@@ -246,7 +246,6 @@ class EmailTemplateForm(SMSTemplateForm):
 
 
 class ForgotPasswordForm(Form):
-    # email_address = email_address()
     email_address = email_address(gov_user=False)
 
 
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'
             ' <a href="{}">contact us</a>').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/utils.py b/app/utils.py
index 031ad7b1c..3908f9c86 100644
--- a/app/utils.py
+++ b/app/utils.py
@@ -203,7 +203,7 @@ def get_help_argument():
     return request.args.get('help') if request.args.get('help') in ('1', '2', '3') else None
 
 
-def user_in_whitelist(email_address):
-    valid_domains = current_app.config.get('EMAIL_DOMAIN_REGEXES', [])
-    email_regex = "[^\@^\s]+@([^@^\\.^\\s]+\.)*({})$".format("|".join(valid_domains))
-    return bool(re.match(email_regex, email_address))
+def is_gov_user(email_address):
+    valid_domains = current_app.config['EMAIL_DOMAIN_REGEXES']
+    email_regex = "[.|@]({})$".format("|".join(valid_domains))
+    return bool(re.search(email_regex, email_address.lower()))
diff --git a/config.py b/config.py
index 837a27fae..2aa355a09 100644
--- a/config.py
+++ b/config.py
@@ -44,23 +44,23 @@ 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",
+        "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"
     ]
 
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))
 

From 2676ee9bcf66280d361e82434710fc40d0166b72 Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Fri, 28 Oct 2016 10:47:32 +0100
Subject: [PATCH 13/22] Add additional tests and refactor

---
 app/main/views/add_service.py           | 58 +++++++++++++++----------
 app/main/views/choose_service.py        |  8 ++--
 app/templates/views/choose-service.html | 12 ++---
 3 files changed, 43 insertions(+), 35 deletions(-)

diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py
index 7cc588e54..3f8a5e064 100644
--- a/app/main/views/add_service.py
+++ b/app/main/views/add_service.py
@@ -1,5 +1,3 @@
-import re
-
 from flask import (
     render_template,
     redirect,
@@ -27,34 +25,49 @@ from app import (
 
 from app.utils import (
     email_safe,
-    user_in_whitelist
+    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'])
 @login_required
 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))
@@ -73,11 +86,8 @@ def add_service():
             help=1
         ))
     else:
-        if not user_in_whitelist(current_user.email_address):
-            abort(403)
-        else:
-            return render_template(
-                'views/add-service.html',
-                form=form,
-                heading=heading
-            )
+        return render_template(
+            'views/add-service.html',
+            form=form,
+            heading=heading
+        )
diff --git a/app/main/views/choose_service.py b/app/main/views/choose_service.py
index 48bf81e39..d5cd2e6b9 100644
--- a/app/main/views/choose_service.py
+++ b/app/main/views/choose_service.py
@@ -1,11 +1,9 @@
-import re
-
-from flask import (current_app, render_template, redirect, url_for, session)
+from flask import (render_template, redirect, url_for, session)
 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 user_in_whitelist
+from app.utils import is_gov_user
 
 
 @main.route("/services")
@@ -15,7 +13,7 @@ def choose_service():
         'views/choose-service.html',
         services=[ServicesBrowsableItem(x) for x in
                   service_api_client.get_services({'user_id': current_user.id})['data']],
-        can_add_service=user_in_whitelist(current_user.email_address)
+        can_add_service=is_gov_user(current_user.email_address)
     )
 
 
diff --git a/app/templates/views/choose-service.html b/app/templates/views/choose-service.html
index f55bda167..fbc55ba4d 100644
--- a/app/templates/views/choose-service.html
+++ b/app/templates/views/choose-service.html
@@ -13,12 +13,12 @@
 
   {{ browse_list(services) }}
   {% if can_add_service %}
-  {{ browse_list([
-    {
-      'title': 'Add a new service…',
-      'link': url_for('.add_service')
-    },
-  ]) }}
+    {{ browse_list([
+      {
+        'title': 'Add a new service…',
+        'link': url_for('.add_service')
+      },
+    ]) }}
   {% endif %}
 
 

From a7e528507334c739e35cc59694f6fd013de126a6 Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Fri, 28 Oct 2016 10:48:29 +0100
Subject: [PATCH 14/22] Add tests to ensure non gov user cannot see, access or
 create service

---
 tests/app/main/views/test_add_service.py  | 25 +++++++++++++-----
 tests/app/main/views/test_all_services.py | 11 ++++++++
 tests/app/main/views/test_manage_users.py |  8 +++---
 tests/conftest.py                         | 31 +++++++++++++++++++++++
 4 files changed, 65 insertions(+), 10 deletions(-)

diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py
index 3947ce013..4dd440dcc 100644
--- a/tests/app/main/views/test_add_service.py
+++ b/tests/app/main/views/test_add_service.py
@@ -1,8 +1,7 @@
 from flask import url_for, session
 from unittest.mock import ANY
 import app
-from app.utils import user_in_whitelist
-from tests.conftest import api_user_active as create_active_user
+from app.utils import is_gov_user
 
 
 def test_get_should_render_add_service_template(app_,
@@ -105,9 +104,23 @@ def test_should_return_form_errors_with_duplicate_service_name_regardless_of_cas
             assert not mock_create_service.called
 
 
-def test_non_whitelist_user_cannot_add_service(app_, mocker, client, fake_uuid):
-    non_whitelist_user = create_active_user(fake_uuid, 'someuser@notonwhitelist.com')
-    client.login(non_whitelist_user, mocker)
-    assert not user_in_whitelist(non_whitelist_user.email_address)
+def test_non_whitelist_user_cannot_access_create_service_page(app_,
+                                                              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(app_,
+                                                  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..09cc543f7 100644
--- a/tests/app/main/views/test_all_services.py
+++ b/tests/app/main/views/test_all_services.py
@@ -30,6 +30,17 @@ 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(app_,
+                                                    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_manage_users.py b/tests/app/main/views/test_manage_users.py
index 87e04944f..2db57d61d 100644
--- a/tests/app/main/views/test_manage_users.py
+++ b/tests/app/main/views/test_manage_users.py
@@ -3,7 +3,7 @@ from flask import url_for
 from bs4 import BeautifulSoup
 import app
 from app.notify_client.models import InvitedUser
-from app.utils import user_in_whitelist
+from app.utils import is_gov_user
 from tests.conftest import service_one as create_sample_service
 
 
@@ -131,7 +131,7 @@ def test_should_show_page_for_inviting_user(
         assert response.status_code == 200
 
 
-@pytest.mark.parametrize('email_address, whitelist_user', [
+@pytest.mark.parametrize('email_address, gov_user', [
     ('test@example.gov.uk', True),
     ('test@nonwhitelist.com', False)
 ])
@@ -141,7 +141,7 @@ def test_invite_user(
         mocker,
         sample_invite,
         email_address,
-        whitelist_user
+        gov_user
 ):
     service = create_sample_service(active_user_with_permissions)
     sample_invite['email_address'] = 'test@example.gov.uk'
@@ -150,7 +150,7 @@ def test_invite_user(
     with app_.test_request_context():
         with app_.test_client() as client:
             client.login(active_user_with_permissions, mocker, service)
-            assert user_in_whitelist(email_address) == whitelist_user
+            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))
diff --git a/tests/conftest.py b/tests/conftest.py
index 4e13cf044..5a110ff71 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -505,6 +505,24 @@ def api_user_active(fake_uuid, email_address='test@user.gov.uk'):
     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,
+                 'permissions': {},
+                 'platform_admin': False,
+                 'password_changed_at': str(datetime.utcnow())
+                 }
+    user = User(user_data)
+    return user
+
+
 @pytest.fixture(scope='function')
 def active_user_with_permissions(fake_uuid):
     from app.notify_client.user_api_client import User
@@ -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:

From e58b63f50412ef6a07ac565168120828933f9c29 Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Fri, 28 Oct 2016 11:44:35 +0100
Subject: [PATCH 15/22] Update regex/config and remove unused imports

---
 app/utils.py                              |  2 +-
 config.py                                 | 34 +++++++++++------------
 tests/app/main/views/test_add_service.py  |  6 ++--
 tests/app/main/views/test_all_services.py |  3 +-
 4 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/app/utils.py b/app/utils.py
index 3908f9c86..74767683d 100644
--- a/app/utils.py
+++ b/app/utils.py
@@ -205,5 +205,5 @@ def get_help_argument():
 
 def is_gov_user(email_address):
     valid_domains = current_app.config['EMAIL_DOMAIN_REGEXES']
-    email_regex = "[.|@]({})$".format("|".join(valid_domains))
+    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 2aa355a09..837a27fae 100644
--- a/config.py
+++ b/config.py
@@ -44,23 +44,23 @@ 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",
+        "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"
     ]
 
diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py
index 4dd440dcc..f3d492cf9 100644
--- a/tests/app/main/views/test_add_service.py
+++ b/tests/app/main/views/test_add_service.py
@@ -104,8 +104,7 @@ def test_should_return_form_errors_with_duplicate_service_name_regardless_of_cas
             assert not mock_create_service.called
 
 
-def test_non_whitelist_user_cannot_access_create_service_page(app_,
-                                                              client,
+def test_non_whitelist_user_cannot_access_create_service_page(client,
                                                               mock_login,
                                                               mock_get_non_govuser,
                                                               api_nongov_user_active):
@@ -115,8 +114,7 @@ def test_non_whitelist_user_cannot_access_create_service_page(app_,
     assert response.status_code == 403
 
 
-def test_non_whitelist_user_cannot_create_service(app_,
-                                                  client,
+def test_non_whitelist_user_cannot_create_service(client,
                                                   mock_login,
                                                   mock_get_non_govuser,
                                                   api_nongov_user_active):
diff --git a/tests/app/main/views/test_all_services.py b/tests/app/main/views/test_all_services.py
index 09cc543f7..d0931558b 100644
--- a/tests/app/main/views/test_all_services.py
+++ b/tests/app/main/views/test_all_services.py
@@ -30,8 +30,7 @@ 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(app_,
-                                                    client,
+def test_non_gov_user_cannot_see_add_service_button(client,
                                                     mock_login,
                                                     mock_get_non_govuser,
                                                     api_nongov_user_active):

From f3a4432ed751a70abdbcbb17138266b08ff56062 Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Fri, 28 Oct 2016 11:45:05 +0100
Subject: [PATCH 16/22] Stop non-gov user seeing/changing email and add test

---
 app/main/views/user_profile.py            | 11 ++++++++++-
 app/templates/views/user-profile.html     |  8 +++++++-
 tests/app/main/views/test_user_profile.py | 20 ++++++++++++++++++++
 3 files changed, 37 insertions(+), 2 deletions(-)

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/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') %}
-      <a href="{{ item.url }}">Change</a>
+      {% if item.label == 'Email address' %}
+        {% if can_see_edit %}
+          <a href="{{ item.url }}">Change</a>
+        {% endif %}
+      {% else %}
+          <a href="{{ item.url }}">Change</a>
+      {% endif %}
     {% endcall %}
   {% endcall %}
 
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 '<a href="/user-profile/email">' 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

From 4dbae97fd14fa95860aab9099ee22141233b4b6b Mon Sep 17 00:00:00 2001
From: Imdad Ahad <imdad.ahad@digital.cabinet-office.gov.uk>
Date: Fri, 28 Oct 2016 12:09:08 +0100
Subject: [PATCH 17/22] Update to use raw strings

---
 config.py | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/config.py b/config.py
index 837a27fae..c230a8041 100644
--- a/config.py
+++ b/config.py
@@ -44,24 +44,24 @@ 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"
     ]
 
 

From a5d228d83762ebed512bf16b50c71db0b434240a Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leo.hemsted@digital.cabinet-office.gov.uk>
Date: Thu, 27 Oct 2016 17:31:13 +0100
Subject: [PATCH 18/22] ensure robustness of email_safe function

* remove leading, trailing, or consecutive periods
* strip unicode accents, umlauts, diacritics etc
---
 app/utils.py            | 15 +++++++++++----
 tests/app/test_utils.py | 19 ++++++++++++++-----
 2 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/app/utils.py b/app/utils.py
index 7610f9ec8..3da4547d3 100644
--- a/app/utils.py
+++ b/app/utils.py
@@ -3,8 +3,11 @@ import csv
 from io import StringIO
 from os import path
 from functools import wraps
+import unicodedata
+
 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
@@ -141,10 +144,14 @@ def generate_previous_next_dict(view, service_id, page, title, url_args):
 
 
 def email_safe(string, whitespace='.'):
-    return "".join([
-        character.lower() if character.isalnum() or character == whitespace else ""
-        for character in re.sub(r"\s+", whitespace, string.strip())
-    ])
+    # strips accents, diacritics etc
+    string = ''.join(c for c in unicodedata.normalize('NFD', string) if unicodedata.category(c) != 'Mn')
+    string = ''.join(
+        word.lower() if word.isalnum() or word == whitespace else ''
+        for word in re.sub(r'\s+', whitespace, string.strip())
+    )
+    string = re.sub(r'\.{2,}', '.', string)
+    return string.strip('.')
 
 
 class Spreadsheet():
diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py
index be62799e4..201726206 100644
--- a/tests/app/test_utils.py
+++ b/tests/app/test_utils.py
@@ -8,11 +8,20 @@ from freezegun import freeze_time
 from app.utils import email_safe, generate_notifications_csv, generate_previous_dict, generate_next_dict, Spreadsheet
 
 
-def test_email_safe_return_dot_separated_email_domain():
-    test_name = 'SOME service  with+stuff+ b123'
-    expected = 'some.service.withstuff.b123'
-    actual = email_safe(test_name)
-    assert actual == expected
+@pytest.mark.parametrize('service_name, safe_email', [
+    ('name with spaces', 'name.with.spaces'),
+    ('singleword', 'singleword'),
+    ('UPPER CASE', 'upper.case'),
+    ('Service - with dash', 'service.with.dash'),
+    ('lots      of spaces', 'lots.of.spaces'),
+    ('name.with.dots', 'name.with.dots'),
+    ('name-with-other-delimiters', 'namewithotherdelimiters'),
+    ('.leading', 'leading'),
+    ('trailing.', 'trailing'),
+    ('üńïçödë wördś', 'unicode.words'),
+])
+def test_email_safe_return_dot_separated_email_domain(service_name, safe_email):
+    assert email_safe(service_name) == safe_email
 
 
 @pytest.mark.parametrize(

From c6291a614ea52de8612e512f711f2cb8fb6baff3 Mon Sep 17 00:00:00 2001
From: Leo Hemsted <leohemsted@gmail.com>
Date: Fri, 28 Oct 2016 16:16:26 +0100
Subject: [PATCH 19/22] Update request-to-go-live.html

fix typo
---
 app/templates/views/service-settings/request-to-go-live.html | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/templates/views/service-settings/request-to-go-live.html b/app/templates/views/service-settings/request-to-go-live.html
index 57ac237d9..d64bc2fa5 100644
--- a/app/templates/views/service-settings/request-to-go-live.html
+++ b/app/templates/views/service-settings/request-to-go-live.html
@@ -20,7 +20,7 @@
       {% endcall %}
 
       <p>
-        Before you request to go live, make you you’ve:
+        Before you request to go live, make sure you’ve:
       </p>
       <ul class="list list-bullet">
         <li>accepted our data sharing and financial agreement</li>

From 14b99e5a24f726bb5caf3549f5069d389a68439d Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 21 Oct 2016 09:39:33 +0100
Subject: [PATCH 20/22] Go to platform admin page when logging in
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If you’re a platform admin, you should go straight to the platform admin
page when you log in.

The all services page is just a crappier version of the same thing,
without all the stats, etc.
---
 app/main/views/two_factor.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/main/views/two_factor.py b/app/main/views/two_factor.py
index 022a189f5..c4d609d64 100644
--- a/app/main/views/two_factor.py
+++ b/app/main/views/two_factor.py
@@ -42,7 +42,7 @@ def two_factor():
             return redirect(next_url)
 
         if current_user.platform_admin:
-            return redirect(url_for('main.show_all_services'))
+            return redirect(url_for('main.platform_admin'))
         if len(services) == 1:
             return redirect(url_for('main.service_dashboard', service_id=services[0]['id']))
         else:

From fc60016566ad8402c5ff57b2671ea5c33f2388ca Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Fri, 21 Oct 2016 09:40:56 +0100
Subject: [PATCH 21/22] =?UTF-8?q?Remove=20=E2=80=98all=20services=E2=80=99?=
 =?UTF-8?q?=20page?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It’s not needed any more, because the platform admin page does the same
thing better.
---
 app/main/__init__.py                   |  1 -
 app/main/views/all_services.py         | 15 ---------------
 tests/app/main/views/test_dashboard.py |  3 ---
 3 files changed, 19 deletions(-)
 delete mode 100644 app/main/views/all_services.py

diff --git a/app/main/__init__.py b/app/main/__init__.py
index b32a6411f..b70ccb942 100644
--- a/app/main/__init__.py
+++ b/app/main/__init__.py
@@ -24,7 +24,6 @@ from app.main.views import (
     api_keys,
     manage_users,
     invites,
-    all_services,
     feedback,
     providers,
     platform_admin
diff --git a/app/main/views/all_services.py b/app/main/views/all_services.py
deleted file mode 100644
index cb1cc8b28..000000000
--- a/app/main/views/all_services.py
+++ /dev/null
@@ -1,15 +0,0 @@
-from flask import render_template
-from flask_login import login_required
-
-from app import service_api_client
-from app.main import main
-from app.utils import user_has_permissions
-from app.notify_client.service_api_client import ServicesBrowsableItem
-
-
-@main.route("/all-services")
-@login_required
-@user_has_permissions(None, admin_override=True)
-def show_all_services():
-    services = [ServicesBrowsableItem(x) for x in service_api_client.get_services()['data']]
-    return render_template('views/all-services.html', services=services)
diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py
index 759777938..09a243e8c 100644
--- a/tests/app/main/views/test_dashboard.py
+++ b/tests/app/main/views/test_dashboard.py
@@ -338,7 +338,6 @@ def test_menu_send_messages(mocker,
 
         assert url_for('main.service_settings', service_id=service_one['id']) not in page
         assert url_for('main.api_keys', service_id=service_one['id']) not in page
-        assert url_for('main.show_all_services') not in page
         assert url_for('main.view_providers') not in page
 
 
@@ -371,7 +370,6 @@ def test_menu_manage_service(mocker,
         assert url_for('main.service_settings', service_id=service_one['id']) in page
 
         assert url_for('main.api_keys', service_id=service_one['id']) not in page
-        assert url_for('main.show_all_services') not in page
 
 
 def test_menu_manage_api_keys(mocker,
@@ -401,7 +399,6 @@ def test_menu_manage_api_keys(mocker,
             template_type='sms') in page
         assert url_for('main.manage_users', service_id=service_one['id']) in page
         assert url_for('main.service_settings', service_id=service_one['id']) not in page
-        assert url_for('main.show_all_services') not in page
 
         assert url_for('main.api_integration', service_id=service_one['id']) in page
 

From 5d7633501519b701626120938393790ff942b778 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Sun, 30 Oct 2016 09:20:53 +0000
Subject: [PATCH 22/22] Move relevant test, delete others
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The test for non-gov.uk domains adding services is still relevant, but
probably makes more sense in `test_add_services.py`.

The others are no longer relevant now the ‘All services’ page has gone.
---
 tests/app/main/views/test_add_service.py  | 10 +++++
 tests/app/main/views/test_all_services.py | 45 -----------------------
 2 files changed, 10 insertions(+), 45 deletions(-)
 delete mode 100644 tests/app/main/views/test_all_services.py

diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py
index f3d492cf9..a1639663b 100644
--- a/tests/app/main/views/test_add_service.py
+++ b/tests/app/main/views/test_add_service.py
@@ -4,6 +4,16 @@ import app
 from app.utils import is_gov_user
 
 
+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 test_get_should_render_add_service_template(app_,
                                                 api_user_active,
                                                 mocker):
diff --git a/tests/app/main/views/test_all_services.py b/tests/app/main/views/test_all_services.py
deleted file mode 100644
index d0931558b..000000000
--- a/tests/app/main/views/test_all_services.py
+++ /dev/null
@@ -1,45 +0,0 @@
-from bs4 import BeautifulSoup
-from flask import url_for
-
-import app
-
-
-def test_all_services_should_render_all_services_template(app_,
-                                                          platform_admin_user,
-                                                          service_one,
-                                                          mocker):
-    with app_.test_request_context():
-        with app_.test_client() as client:
-            _login_user(client, mocker, platform_admin_user, service_one)
-            mocker.patch('app.service_api_client.get_services', return_value={'data': [service_one]})
-            response = client.get(url_for('main.show_all_services'))
-            assert response.status_code == 200
-            page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
-            assert page.h1.string.strip() == 'All services'
-            assert app.service_api_client.get_services.call_count == 1
-
-
-def test_all_service_returns_403_when_not_a_platform_admin(app_,
-                                                           active_user_with_permissions,
-                                                           service_one,
-                                                           mocker):
-    with app_.test_request_context():
-        with app_.test_client() as client:
-            _login_user(client, mocker, active_user_with_permissions, service_one)
-            response = client.get(url_for('main.show_all_services'))
-            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)