From a95d9b51525b96a694a8424b2e253cd94dc767e3 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Wed, 3 Jun 2020 14:54:37 +0100
Subject: [PATCH 1/2] Enforce service permissions

This should catch typos more quickly and obviously.
---
 app/models/service.py            | 12 ++++++++++++
 tests/app/models/test_service.py |  8 ++++++++
 2 files changed, 20 insertions(+)

diff --git a/app/models/service.py b/app/models/service.py
index f536fc64c..dd972940d 100644
--- a/app/models/service.py
+++ b/app/models/service.py
@@ -58,6 +58,16 @@ class Service(JSONModel):
         'letter',
     )
 
+    ALL_PERMISSIONS = TEMPLATE_TYPES + (
+        'edit_folder_permissions',
+        'email_auth',
+        'inbound_sms',
+        'international_letters',
+        'international_sms',
+        'upload_document',
+        'upload_letters',
+    )
+
     def __init__(self, _dict):
 
         super().__init__(_dict)
@@ -106,6 +116,8 @@ class Service(JSONModel):
         return not self.trial_mode
 
     def has_permission(self, permission):
+        if permission not in self.ALL_PERMISSIONS:
+            raise KeyError(f'{permission} is not a service permission')
         return permission in self.permissions
 
     def get_page_of_jobs(self, page):
diff --git a/tests/app/models/test_service.py b/tests/app/models/test_service.py
index 84f9d8377..9c1eaa98d 100644
--- a/tests/app/models/test_service.py
+++ b/tests/app/models/test_service.py
@@ -1,5 +1,7 @@
 import uuid
 
+import pytest
+
 from app.models.organisation import Organisation
 from app.models.service import Service
 from app.models.user import User
@@ -247,3 +249,9 @@ def test_service_without_organisation_doesnt_need_org_api(mocker, service_one):
 
     assert mock_redis_get.called is False
     assert mock_get_organisation.called is False
+
+
+def test_bad_permission_raises(service_one):
+    with pytest.raises(KeyError) as e:
+        Service(service_one).has_permission('foo')
+    assert str(e.value) == "'foo is not a service permission'"

From e089de74ee6445aeed4296bdade6c54aa4b1ef4f Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Wed, 3 Jun 2020 15:04:45 +0100
Subject: [PATCH 2/2] Refactor to not need custom constructor

All the constructor of the service model is doing is setting a default
value of a property, this is more idiomatically expressed with a custom
property, and means we can get rid of the custom constructor entirely.
---
 app/models/service.py | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/app/models/service.py b/app/models/service.py
index dd972940d..2d95e7b29 100644
--- a/app/models/service.py
+++ b/app/models/service.py
@@ -39,7 +39,6 @@ class Service(JSONModel):
         'letter_contact_block',
         'message_limit',
         'name',
-        'permissions',
         'prefix_sms',
         'research_mode',
         'service_callback_api',
@@ -68,16 +67,14 @@ class Service(JSONModel):
         'upload_letters',
     )
 
-    def __init__(self, _dict):
-
-        super().__init__(_dict)
-        if 'permissions' not in self._dict:
-            self.permissions = {'email', 'sms', 'letter'}
-
     @classmethod
     def from_id(cls, service_id):
         return cls(service_api_client.get_service(service_id)['data'])
 
+    @property
+    def permissions(self):
+        return self._dict.get('permissions', self.TEMPLATE_TYPES)
+
     def update(self, **kwargs):
         return service_api_client.update_service(self.id, **kwargs)