From 34ed0da3622cefb7992fa987dd505d7a6cf2e735 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Thu, 6 Sep 2018 12:12:36 +0100
Subject: [PATCH] Validate non canonical domains
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

At the moment we transform what the user gives us, so if someone enters
`digital.cabinet-office.gov.uk` it will automagically be saved as
`cabinet-office.gov.uk`. This happens without the user knowing, and
might have unintended consequences.

So let’s tell them what the problem is, and let them decide what to do
about it (which might be accepting the canonical domain, or adding a
new organisation to domains.yml first).
---
 app/main/forms.py                           |  6 +++-
 app/main/validators.py                      | 22 ++++++++++++
 tests/app/main/views/test_email_branding.py | 38 +++++++++++++--------
 3 files changed, 50 insertions(+), 16 deletions(-)

diff --git a/app/main/forms.py b/app/main/forms.py
index 536a9b672..1106bc12f 100644
--- a/app/main/forms.py
+++ b/app/main/forms.py
@@ -33,6 +33,7 @@ from wtforms.validators import URL, DataRequired, Length, Optional, Regexp
 
 from app.main.validators import (
     Blacklist,
+    CanonicalGovernmentDomain,
     CsvFileValidator,
     DoesNotStartWithDoubleZero,
     KnownGovernmentDomain,
@@ -721,7 +722,10 @@ class ServicePreviewBranding(StripWhitespaceForm):
 
 
 class GovernmentDomainField(StringField):
-    validators = [KnownGovernmentDomain()]
+    validators = [
+        KnownGovernmentDomain(),
+        CanonicalGovernmentDomain(),
+    ]
 
     def post_validate(self, form, validation_stopped):
         if self.data and not self.errors:
diff --git a/app/main/validators.py b/app/main/validators.py
index 3067e46c0..ea24bb95f 100644
--- a/app/main/validators.py
+++ b/app/main/validators.py
@@ -111,3 +111,25 @@ class KnownGovernmentDomain:
     def __call__(self, form, field):
         if field.data and AgreementInfo(field.data).owner is None:
             raise ValidationError(self.message)
+
+
+class CanonicalGovernmentDomain:
+
+    message = 'Not {} domain (use {} if appropriate)'
+
+    def __call__(self, form, field):
+
+        if not field.data:
+            return
+
+        domain = AgreementInfo(field.data)
+
+        if not domain.is_canonical:
+            raise ValidationError(
+                self.message.format('a canonical', domain.canonical_domain)
+            )
+
+        if field.data != domain.canonical_domain:
+            raise ValidationError(
+                self.message.format('an organisation-level', domain.canonical_domain)
+            )
diff --git a/tests/app/main/views/test_email_branding.py b/tests/app/main/views/test_email_branding.py
index 797f5a8e5..e190b0d30 100644
--- a/tests/app/main/views/test_email_branding.py
+++ b/tests/app/main/views/test_email_branding.py
@@ -155,18 +155,31 @@ def test_cant_create_new_email_branding_with_unknown_domain(
     )
 
 
-@pytest.mark.parametrize('posted_domain, persisted_domain', [
-    ('voa.gsi.gov.uk', 'voa.gov.uk'),
-    ('voa.gov.uk', 'voa.gov.uk'),
-    ('hmcts.net', 'hmcts.gov.uk'),
+@pytest.mark.parametrize('posted_domain, expected_error', [
+    (
+        'voa.gsi.gov.uk',
+        'Not a canonical domain (use voa.gov.uk if appropriate)',
+    ),
+    (
+        'hmcts.net',
+        'Not a canonical domain (use hmcts.gov.uk if appropriate)',
+    ),
+    (
+        'southend.essex.gov.uk',
+        'Not an organisation-level domain (use essex.gov.uk if appropriate)',
+    ),
+    pytest.mark.xfail(
+        ('voa.gov.uk', ''),
+        raises=AssertionError
+    ),
 ])
-def test_persists_canonical_domain_when_adding_email_branding(
+def test_rejects_non_canonical_domain_when_adding_email_branding(
     client_request,
     mocker,
     fake_uuid,
     mock_create_email_branding,
     posted_domain,
-    persisted_domain,
+    expected_error,
 ):
     mocker.patch('app.main.views.email_branding.persist_logo')
     mocker.patch('app.main.views.email_branding.delete_temp_files_created_by')
@@ -179,20 +192,15 @@ def test_persists_canonical_domain_when_adding_email_branding(
         'brand_type': 'org',
     }
     client_request.login(platform_admin_user(fake_uuid))
-    client_request.post(
+    page = client_request.post(
         '.create_email_branding',
         content_type='multipart/form-data',
         _data=data,
+        _expected_status=200,
     )
 
-    assert mock_create_email_branding.call_args == call(
-        logo=data['logo'],
-        name=data['name'],
-        text=data['text'],
-        colour=data['colour'],
-        domain=persisted_domain,
-        brand_type=data['brand_type']
-    )
+    assert page.select_one('.error-message').text.strip() == expected_error
+    assert mock_create_email_branding.called is False
 
 
 def test_create_new_email_branding_when_branding_saved(