Merge pull request #984 from alphagov/ken-update-api-service-permissions-handling

Ken update api service permissions handling

app/dao/services_dao.py

@@ -31,7 +31,9 @@ from app.models import (
     TEMPLATE_TYPES,
     JobStatistics,
     SMS_TYPE,
-    EMAIL_TYPE
+    EMAIL_TYPE,
+    INTERNATIONAL_SMS_TYPE,
+    LETTER_TYPE
 )
 from app.service.statistics import format_monthly_template_notification_stats
 from app.statsd_decorators import statsd
@@ -136,10 +138,17 @@ def dao_create_service(service, user, service_id=None, service_permissions=[SMS_
     service.active = True
     service.research_mode = False
 
-    for permission in service_permissions:
+    def deprecate_process_service_permissions():
-        service_permission = ServicePermission(service_id=service.id, permission=permission)
+        for permission in service_permissions:
-        db.session.add(service_permission)
+            service_permission = ServicePermission(service_id=service.id, permission=permission)
+            service.permissions.append(service_permission)
 
+            if permission == INTERNATIONAL_SMS_TYPE:
+                service.can_send_international_sms = True
+            if permission == LETTER_TYPE:
+                service.can_send_letters = True
+
+    deprecate_process_service_permissions()
     db.session.add(service)
 
 

app/models.py

@@ -241,7 +241,8 @@ class ServicePermission(db.Model):
     service = db.relationship("Service")
     created_at = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False)
 
-    service_permission_types = db.relationship(Service, backref=db.backref("permissions"))
+    service_permission_types = db.relationship(
+        Service, backref=db.backref("permissions", cascade="all, delete-orphan"))
 
     def __repr__(self):
         return '<{} has service permission: {}>'.format(self.service_id, self.permission)

app/schemas.py

@@ -25,7 +25,9 @@ from notifications_utils.recipients import (
 
 from app import ma
 from app import models
+from app.models import ServicePermission, INTERNATIONAL_SMS_TYPE, SMS_TYPE, LETTER_TYPE, EMAIL_TYPE
 from app.dao.permissions_dao import permission_dao
+from app.dao.service_permissions_dao import dao_fetch_service_permissions
 from app.utils import get_template_instance
 
 
@@ -178,18 +180,57 @@ class ServiceSchema(BaseSchema):
     organisation = field_for(models.Service, 'organisation')
     branding = field_for(models.Service, 'branding')
     dvla_organisation = field_for(models.Service, 'dvla_organisation')
+    permissions = fields.Method("service_permissions")
+    override_flag = False
+
+    def service_permissions(self, service):
+        permissions = []
+        str_permissions = []
+
+        perms = dao_fetch_service_permissions(service.id)
+        for p in perms:
+            permission = {
+                "service_id": service.id,
+                "permission": p.permission
+            }
+            permissions.append(permission)
+            str_permissions.append(p.permission)
+
+        def deprecate_convert_flags_to_permissions():
+            def convert_flags(flag, notify_type):
+                if flag and notify_type not in str_permissions:
+                    permission = {
+                        "service_id": service.id,
+                        "permission": notify_type
+                    }
+                    permissions.append(permission)
+                elif flag is False and notify_type in str_permissions:
+                    permission = {
+                        "service_id": service.id,
+                        "permission": notify_type
+                    }
+                    permissions.remove(permission)
+
+            convert_flags(service.can_send_international_sms, INTERNATIONAL_SMS_TYPE)
+            convert_flags(service.can_send_letters, LETTER_TYPE)
+
+        deprecate_convert_flags_to_permissions()
+
+        return permissions
 
     class Meta:
         model = models.Service
-        exclude = ('updated_at',
+        exclude = (
-                   'created_at',
+            'updated_at',
-                   'api_keys',
+            'created_at',
-                   'templates',
+            'api_keys',
-                   'jobs',
+            'templates',
-                   'old_id',
+            'jobs',
-                   'template_statistics',
+            'old_id',
-                   'service_provider_stats',
+            'template_statistics',
-                   'service_notification_stats')
+            'service_provider_stats',
+            'service_notification_stats',
+        )
         strict = True
 
     @validates('sms_sender')
@@ -197,6 +238,58 @@ class ServiceSchema(BaseSchema):
         if value and not re.match(r'^[a-zA-Z0-9\s]+$', value):
             raise ValidationError('Only alphanumeric characters allowed')
 
+    @validates('permissions')
+    def validate_permissions(self, value):
+        permissions = [v.permission for v in value]
+        for p in permissions:
+            if p not in models.SERVICE_PERMISSION_TYPES:
+                raise ValidationError("Invalid Service Permission: '{}'".format(p))
+
+        if len(set(permissions)) != len(permissions):
+            duplicates = list(set([x for x in permissions if permissions.count(x) > 1]))
+            raise ValidationError('Duplicate Service Permission: {}'.format(duplicates))
+
+    @pre_load()
+    def format_for_data_model(self, in_data):
+        if isinstance(in_data, dict) and 'permissions' in in_data:
+            str_permissions = in_data['permissions']
+            permissions = []
+            for p in in_data['permissions']:
+                permission = ServicePermission(service_id=in_data["id"], permission=p)
+                permissions.append(permission)
+            in_data['permissions'] = permissions
+
+            def deprecate_override_flags():
+                in_data['can_send_letters'] = LETTER_TYPE in [p.permission for p in permissions]
+                in_data['can_send_international_sms'] = INTERNATIONAL_SMS_TYPE in [p.permission for p in permissions]
+
+            def deprecate_convert_flags_to_permissions():
+                def convert_flags(flag, notify_type):
+                    if flag and notify_type not in str_permissions:
+                        permission = ServicePermission(service_id=in_data['id'], permission=notify_type)
+                        permissions.append(permission)
+                    elif flag is False and notify_type in str_permissions:
+                        for p in permissions:
+                            if p.permission == notify_type:
+                                permissions.remove(p)
+
+                convert_flags(in_data["can_send_international_sms"], INTERNATIONAL_SMS_TYPE)
+                convert_flags(in_data["can_send_letters"], LETTER_TYPE)
+
+            if self.override_flag:
+                deprecate_override_flags()
+            else:
+                deprecate_convert_flags_to_permissions()
+
+    @post_dump
+    def format_as_string_array(self, in_data):
+        if isinstance(in_data, dict) and 'permissions' in in_data:
+            in_data['permissions'] = [p.get("permission") for p in in_data['permissions']]
+        return in_data
+
+    def set_override_flag(self, flag):
+        self.override_flag = flag
+
 
 class DetailedServiceSchema(BaseSchema):
     statistics = fields.Dict()

app/service/rest.py

@@ -95,12 +95,11 @@ def get_services():
 def get_service_by_id(service_id):
     if request.args.get('detailed') == 'True':
         data = get_detailed_service(service_id, today_only=request.args.get('today_only') == 'True')
-        return jsonify(data=data)
     else:
         fetched = dao_fetch_service_by_id(service_id)
 
         data = service_schema.dump(fetched).data
-        return jsonify(data=data)
+    return jsonify(data=data)
 
 
 @service_blueprint.route('', methods=['POST'])
@@ -129,6 +128,7 @@ def update_service(service_id):
     service_going_live = fetched_service.restricted and not request.get_json().get('restricted', True)
 
     current_data = dict(service_schema.dump(fetched_service).data.items())
+    service_schema.set_override_flag(request.get_json().get('permissions') is not None)
     current_data.update(request.get_json())
     update_dict = service_schema.load(current_data).data
     dao_update_service(update_dict)

tests/app/conftest.py

@@ -24,7 +24,7 @@ from app.models import (
     NotificationStatistics,
     ServiceWhitelist,
     KEY_TYPE_NORMAL, KEY_TYPE_TEST, KEY_TYPE_TEAM,
-    MOBILE_TYPE, EMAIL_TYPE, LETTER_TYPE, NOTIFICATION_STATUS_TYPES_COMPLETED, ScheduledNotification)
+    MOBILE_TYPE, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, NOTIFICATION_STATUS_TYPES_COMPLETED, ScheduledNotification)
 from app.dao.users_dao import (create_user_code, create_secret_code)
 from app.dao.services_dao import (dao_create_service, dao_add_user_to_service)
 from app.dao.templates_dao import dao_create_template
@@ -124,7 +124,8 @@ def sample_service(
     restricted=False,
     limit=1000,
     email_from=None,
-    can_send_international_sms=False
+    can_send_international_sms=False,
+    permissions=[SMS_TYPE, EMAIL_TYPE]
 ):
     if user is None:
         user = create_user()
@@ -142,7 +143,7 @@ def sample_service(
     service = Service.query.filter_by(name=service_name).first()
     if not service:
         service = Service(**data)
-        dao_create_service(service, user)
+        dao_create_service(service, user, service_permissions=permissions)
     else:
         if user not in service.users:
             dao_add_user_to_service(service, user)

tests/app/dao/test_services_dao.py

@@ -258,15 +258,15 @@ def test_create_service_returns_service_with_default_permissions(service_factory
 
     service = dao_fetch_service_by_id(service.id)
     assert len(service.permissions) == 2
-    assert all(p.permission in [SMS_TYPE, EMAIL_TYPE] for p in service.permissions)
+    assert set([SMS_TYPE, EMAIL_TYPE]) == set(p.permission for p in service.permissions)
 
 
 # This test is only for backward compatibility and will be removed
-# when the 'can_use' columns are dropped from the Service data model
+# when the deprecated 'can_use' columns are not used in the Service data model
 @pytest.mark.parametrize("permission_to_add, can_send_letters, can_send_international_sms",
                          [(LETTER_TYPE, True, False),
                           (INTERNATIONAL_SMS_TYPE, False, True)])
-def test_create_service_by_id_adding_service_permission_returns_service_with_permissions_set(
+def test_create_service_by_id_adding_service_permission_returns_service_with_flags_and_permissions_set(
         service_factory, permission_to_add, can_send_letters, can_send_international_sms):
     service = service_factory.get('testing', email_from='testing')
 
@@ -275,18 +275,59 @@ def test_create_service_by_id_adding_service_permission_returns_service_with_per
 
     service = dao_fetch_service_by_id(service.id)
     assert len(service.permissions) == 3
-    assert all(p.permission in [SMS_TYPE, EMAIL_TYPE, permission_to_add] for p in service.permissions)
+    assert set([SMS_TYPE, EMAIL_TYPE, permission_to_add]) == set(p.permission for p in service.permissions)
     assert service.can_send_letters == can_send_letters
     assert service.can_send_international_sms == can_send_international_sms
 
 
-def test_remove_permission_from_service_by_id_returns_service_with_correct_permissions(service_factory):
+# This test is only for backward compatibility and will be removed
+# when the deprecated 'can_use' columns are not used in the Service data model
+@pytest.mark.parametrize("permission_to_remove, can_send_letters, can_send_international_sms",
+                         [(LETTER_TYPE, False, True),
+                          (INTERNATIONAL_SMS_TYPE, True, False)])
+def test_create_service_by_id_removing_service_permission_returns_service_with_flags_and_permissions_set(
+        service_factory, permission_to_remove, can_send_letters, can_send_international_sms):
     service = service_factory.get('testing', email_from='testing')
-    dao_remove_service_permission(service_id=service.id, permission=SMS_TYPE)
+
+    dao_add_service_permission(service_id=service.id, permission=LETTER_TYPE)
+    dao_add_service_permission(service_id=service.id, permission=INTERNATIONAL_SMS_TYPE)
+    service = dao_fetch_service_by_id(service.id)
+    service.set_permissions()
+    assert len(service.permissions) == 4
+    assert service.can_send_letters
+    assert service.can_send_international_sms
+
+    dao_remove_service_permission(service_id=service.id, permission=permission_to_remove)
+    service.set_permissions()
 
     service = dao_fetch_service_by_id(service.id)
+    expected_permissions = [SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE]
+    expected_permissions.remove(permission_to_remove)
+
+    assert len(service.permissions) == 3
+    assert set(expected_permissions) == set(p.permission for p in service.permissions)
+    assert service.can_send_letters == can_send_letters
+    assert service.can_send_international_sms == can_send_international_sms
+
+
+@pytest.mark.parametrize("permission_to_remove, permission_remaining",
+                         [(SMS_TYPE, EMAIL_TYPE),
+                          (EMAIL_TYPE, SMS_TYPE)])
+def test_remove_permission_from_service_by_id_returns_service_with_correct_permissions(
+        sample_service, permission_to_remove, permission_remaining):
+    dao_remove_service_permission(service_id=sample_service.id, permission=permission_to_remove)
+
+    service = dao_fetch_service_by_id(sample_service.id)
     assert len(service.permissions) == 1
-    assert service.permissions[0].permission == EMAIL_TYPE
+    assert service.permissions[0].permission == permission_remaining
+
+
+def test_removing_all_permission_returns_service_with_no_permissions(sample_service):
+    dao_remove_service_permission(service_id=sample_service.id, permission=SMS_TYPE)
+    dao_remove_service_permission(service_id=sample_service.id, permission=EMAIL_TYPE)
+
+    service = dao_fetch_service_by_id(sample_service.id)
+    assert len(service.permissions) == 0
 
 
 def test_remove_service_does_not_remove_service_permission_types(sample_service):
@@ -294,7 +335,7 @@ def test_remove_service_does_not_remove_service_permission_types(sample_service)
 
     services = dao_fetch_all_services()
     assert len(services) == 0
-    assert set([p.name for p in ServicePermissionTypes.query.all()]) & set(SERVICE_PERMISSION_TYPES)
+    assert set(p.name for p in ServicePermissionTypes.query.all()) == set(SERVICE_PERMISSION_TYPES)
 
 
 def test_create_service_by_id_adding_and_removing_letter_returns_service_without_letter(service_factory):
@@ -372,6 +413,42 @@ def test_update_service_creates_a_history_record_with_current_data(sample_user):
     assert Service.get_history_model().query.filter_by(name='updated_service_name').one().version == 2
 
 
+def test_update_service_permission_creates_a_history_record_with_current_data(sample_user):
+    assert Service.query.count() == 0
+    assert Service.get_history_model().query.count() == 0
+    service = Service(name="service_name",
+                      email_from="email_from",
+                      message_limit=1000,
+                      restricted=False,
+                      created_by=sample_user)
+    dao_create_service(service, sample_user)
+
+    service.permissions.append(ServicePermission(service_id=service.id, permission='letter'))
+    dao_update_service(service)
+
+    assert Service.query.count() == 1
+    assert Service.get_history_model().query.count() == 2
+
+    service_from_db = Service.query.first()
+
+    assert service_from_db.version == 2
+    assert LETTER_TYPE in [p.permission for p in service_from_db.permissions]
+
+    permission = [p for p in service.permissions if p.permission == 'sms'][0]
+    service.permissions.remove(permission)
+    dao_update_service(service)
+
+    assert Service.query.count() == 1
+    assert Service.get_history_model().query.count() == 3
+
+    service_from_db = Service.query.first()
+    assert service_from_db.version == 3
+    assert SMS_TYPE not in [p.permission for p in service_from_db.permissions]
+
+    assert len(Service.get_history_model().query.filter_by(name='service_name').all()) == 3
+    assert Service.get_history_model().query.filter_by(name='service_name').all()[2].version == 3
+
+
 def test_create_service_and_history_is_transactional(sample_user):
     assert Service.query.count() == 0
     assert Service.get_history_model().query.count() == 0

tests/app/service/test_rest.py

@@ -10,7 +10,7 @@ from freezegun import freeze_time
 
 from app.dao.users_dao import save_model_user
 from app.dao.services_dao import dao_remove_user_from_service
-from app.models import User, Organisation, DVLA_ORG_LAND_REGISTRY, Rate
+from app.models import User, Organisation, DVLA_ORG_LAND_REGISTRY, Rate, ServicePermission
 from tests import create_authorization_header
 from tests.app.db import create_template
 from tests.app.conftest import (
@@ -20,28 +20,30 @@ from tests.app.conftest import (
     sample_notification_history as create_notification_history,
     sample_notification_with_job
 )
-from app.models import Service, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST
+from app.models import (
+    Service, ServicePermission,
+    KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST,
+    EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE, INBOUND_SMS_TYPE
+)
 
 from tests.app.db import create_user
 
 
-def test_get_service_list(notify_api, service_factory):
+def test_get_service_list(client, service_factory):
-    with notify_api.test_request_context():
+    service_factory.get('one')
-        with notify_api.test_client() as client:
+    service_factory.get('two')
-            service_factory.get('one')
+    service_factory.get('three')
-            service_factory.get('two')
+    auth_header = create_authorization_header()
-            service_factory.get('three')
+    response = client.get(
-            auth_header = create_authorization_header()
+        '/service',
-            response = client.get(
+        headers=[auth_header]
-                '/service',
+    )
-                headers=[auth_header]
+    assert response.status_code == 200
-            )
+    json_resp = json.loads(response.get_data(as_text=True))
-            assert response.status_code == 200
+    assert len(json_resp['data']) == 3
-            json_resp = json.loads(response.get_data(as_text=True))
+    assert json_resp['data'][0]['name'] == 'one'
-            assert len(json_resp['data']) == 3
+    assert json_resp['data'][1]['name'] == 'two'
-            assert json_resp['data'][0]['name'] == 'one'
+    assert json_resp['data'][2]['name'] == 'three'
-            assert json_resp['data'][1]['name'] == 'two'
-            assert json_resp['data'][2]['name'] == 'three'
 
 
 def test_get_service_list_with_only_active_flag(client, service_factory):
@@ -117,17 +119,15 @@ def test_get_service_list_by_user_should_return_empty_list_if_no_services(client
     assert len(json_resp['data']) == 0
 
 
-def test_get_service_list_should_return_empty_list_if_no_services(notify_api, notify_db, notify_db_session):
+def test_get_service_list_should_return_empty_list_if_no_services(client):
-    with notify_api.test_request_context():
+    auth_header = create_authorization_header()
-        with notify_api.test_client() as client:
+    response = client.get(
-            auth_header = create_authorization_header()
+        '/service',
-            response = client.get(
+        headers=[auth_header]
-                '/service',
+    )
-                headers=[auth_header]
+    assert response.status_code == 200
-            )
+    json_resp = json.loads(response.get_data(as_text=True))
-            assert response.status_code == 200
+    assert len(json_resp['data']) == 0
-            json_resp = json.loads(response.get_data(as_text=True))
-            assert len(json_resp['data']) == 0
 
 
 def test_get_service_by_id(client, sample_service):
@@ -147,6 +147,32 @@ def test_get_service_by_id(client, sample_service):
     assert json_resp['data']['sms_sender'] == current_app.config['FROM_NUMBER']
 
 
+def test_get_service_list_has_default_permissions(client, service_factory):
+    service_factory.get('one')
+    service_factory.get('two')
+    service_factory.get('three')
+    auth_header = create_authorization_header()
+    response = client.get(
+        '/service',
+        headers=[auth_header]
+    )
+    assert response.status_code == 200
+    json_resp = json.loads(response.get_data(as_text=True))
+    assert len(json_resp['data']) == 3
+    assert all([set(json['permissions']) == set([EMAIL_TYPE, SMS_TYPE]) for json in json_resp['data']])
+
+
+def test_get_service_by_id_has_default_service_permissions(client, sample_service):
+    auth_header = create_authorization_header()
+    resp = client.get(
+        '/service/{}'.format(sample_service.id),
+        headers=[auth_header]
+    )
+    json_resp = json.loads(resp.get_data(as_text=True))
+
+    assert set(json_resp['data']['permissions']) == set([EMAIL_TYPE, SMS_TYPE])
+
+
 def test_get_service_by_id_should_404_if_no_service(notify_api, notify_db):
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
@@ -414,39 +440,196 @@ def test_update_service(client, notify_db, sample_service):
     assert result['data']['dvla_organisation'] == DVLA_ORG_LAND_REGISTRY
 
 
-def test_update_service_flags(notify_api, sample_service):
+def test_update_service_flags(client, sample_service):
-    with notify_api.test_request_context():
+    auth_header = create_authorization_header()
-        with notify_api.test_client() as client:
+    resp = client.get(
-            auth_header = create_authorization_header()
+        '/service/{}'.format(sample_service.id),
-            resp = client.get(
+        headers=[auth_header]
-                '/service/{}'.format(sample_service.id),
+    )
-                headers=[auth_header]
+    json_resp = json.loads(resp.get_data(as_text=True))
-            )
+    assert resp.status_code == 200
-            json_resp = json.loads(resp.get_data(as_text=True))
+    assert json_resp['data']['name'] == sample_service.name
-            assert resp.status_code == 200
+    assert json_resp['data']['research_mode'] is False
-            assert json_resp['data']['name'] == sample_service.name
+    assert json_resp['data']['can_send_letters'] is False
-            assert json_resp['data']['research_mode'] is False
+    assert json_resp['data']['can_send_international_sms'] is False
-            assert json_resp['data']['can_send_letters'] is False
-            assert json_resp['data']['can_send_international_sms'] is False
 
-            data = {
+    data = {
-                'research_mode': True,
+        'research_mode': True,
-                'can_send_letters': True,
+        'can_send_letters': True,
-                'can_send_international_sms': True,
+        'can_send_international_sms': True,
-            }
+    }
 
-            auth_header = create_authorization_header()
+    auth_header = create_authorization_header()
 
-            resp = client.post(
+    resp = client.post(
-                '/service/{}'.format(sample_service.id),
+        '/service/{}'.format(sample_service.id),
-                data=json.dumps(data),
+        data=json.dumps(data),
-                headers=[('Content-Type', 'application/json'), auth_header]
+        headers=[('Content-Type', 'application/json'), auth_header]
-            )
+    )
-            result = json.loads(resp.get_data(as_text=True))
+    result = json.loads(resp.get_data(as_text=True))
-            assert resp.status_code == 200
+    assert resp.status_code == 200
-            assert result['data']['research_mode'] is True
+    assert result['data']['research_mode'] is True
-            assert result['data']['can_send_letters'] is True
+    assert result['data']['can_send_letters'] is True
-            assert result['data']['can_send_international_sms'] is True
+    assert result['data']['can_send_international_sms'] is True
+
+
+@pytest.fixture(scope='function')
+def service_with_no_permissions(notify_db, notify_db_session):
+    return create_service(notify_db, notify_db_session, permissions=[])
+
+
+def test_update_service_flags_with_service_without_default_service_permissions(client, service_with_no_permissions):
+    auth_header = create_authorization_header()
+    data = {
+        'can_send_letters': True,
+        'can_send_international_sms': True,
+    }
+
+    resp = client.post(
+        '/service/{}'.format(service_with_no_permissions.id),
+        data=json.dumps(data),
+        headers=[('Content-Type', 'application/json'), auth_header]
+    )
+    result = json.loads(resp.get_data(as_text=True))
+
+    assert resp.status_code == 200
+    assert result['data']['can_send_letters'] is True
+    assert result['data']['can_send_international_sms'] is True
+    assert set(result['data']['permissions']) == set([LETTER_TYPE, INTERNATIONAL_SMS_TYPE])
+
+
+def test_update_service_flags_will_remove_service_permissions(client, notify_db, notify_db_session):
+    auth_header = create_authorization_header()
+
+    service = create_service(
+        notify_db, notify_db_session, permissions=[SMS_TYPE, EMAIL_TYPE, INTERNATIONAL_SMS_TYPE])
+
+    assert service.can_send_international_sms is True
+
+    data = {
+        'can_send_international_sms': False
+    }
+
+    resp = client.post(
+        '/service/{}'.format(service.id),
+        data=json.dumps(data),
+        headers=[('Content-Type', 'application/json'), auth_header]
+    )
+    result = json.loads(resp.get_data(as_text=True))
+
+    assert resp.status_code == 200
+    assert result['data']['can_send_international_sms'] is False
+    assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE])
+
+
+def test_update_permissions_will_override_permission_flags(client, service_with_no_permissions):
+    auth_header = create_authorization_header()
+
+    data = {
+        'permissions': [LETTER_TYPE, INTERNATIONAL_SMS_TYPE]
+    }
+
+    resp = client.post(
+        '/service/{}'.format(service_with_no_permissions.id),
+        data=json.dumps(data),
+        headers=[('Content-Type', 'application/json'), auth_header]
+    )
+    result = json.loads(resp.get_data(as_text=True))
+
+    assert resp.status_code == 200
+    assert result['data']['can_send_letters'] is True
+    assert result['data']['can_send_international_sms'] is True
+    assert set(result['data']['permissions']) == set([LETTER_TYPE, INTERNATIONAL_SMS_TYPE])
+
+
+def test_update_service_permissions_will_add_service_permissions(client, sample_service):
+    auth_header = create_authorization_header()
+
+    data = {
+        'permissions': [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE]
+    }
+
+    resp = client.post(
+        '/service/{}'.format(sample_service.id),
+        data=json.dumps(data),
+        headers=[('Content-Type', 'application/json'), auth_header]
+    )
+    result = json.loads(resp.get_data(as_text=True))
+
+    assert resp.status_code == 200
+    assert set(result['data']['permissions']) == set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE])
+
+
+@pytest.mark.parametrize(
+    'permission_to_add',
+    [
+        (EMAIL_TYPE),
+        (SMS_TYPE),
+        (INTERNATIONAL_SMS_TYPE),
+        (LETTER_TYPE),
+        (INBOUND_SMS_TYPE),
+    ]
+)
+def test_add_service_permission_will_add_permission(client, service_with_no_permissions, permission_to_add):
+    auth_header = create_authorization_header()
+
+    data = {
+        'permissions': [permission_to_add]
+    }
+
+    resp = client.post(
+        '/service/{}'.format(service_with_no_permissions.id),
+        data=json.dumps(data),
+        headers=[('Content-Type', 'application/json'), auth_header]
+    )
+
+    resp = client.get(
+        '/service/{}'.format(service_with_no_permissions.id),
+        headers=[auth_header]
+    )
+    result = json.loads(resp.get_data(as_text=True))
+
+    assert resp.status_code == 200
+    assert result['data']['permissions'] == [permission_to_add]
+
+
+def test_update_permissions_with_an_invalid_permission_will_raise_error(client, sample_service):
+    auth_header = create_authorization_header()
+    invalid_permission = 'invalid_permission'
+
+    data = {
+        'permissions': [EMAIL_TYPE, SMS_TYPE, invalid_permission]
+    }
+
+    resp = client.post(
+        '/service/{}'.format(sample_service.id),
+        data=json.dumps(data),
+        headers=[('Content-Type', 'application/json'), auth_header]
+    )
+    result = json.loads(resp.get_data(as_text=True))
+
+    assert resp.status_code == 400
+    assert result['result'] == 'error'
+    assert "Invalid Service Permission: '{}'".format(invalid_permission) in result['message']['permissions']
+
+
+def test_update_permissions_with_duplicate_permissions_will_raise_error(client, sample_service):
+    auth_header = create_authorization_header()
+
+    data = {
+        'permissions': [EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, LETTER_TYPE]
+    }
+
+    resp = client.post(
+        '/service/{}'.format(sample_service.id),
+        data=json.dumps(data),
+        headers=[('Content-Type', 'application/json'), auth_header]
+    )
+    result = json.loads(resp.get_data(as_text=True))
+
+    assert resp.status_code == 400
+    assert result['result'] == 'error'
+    assert "Duplicate Service Permission: ['{}']".format(LETTER_TYPE) in result['message']['permissions']
 
 
 def test_update_service_research_mode_throws_validation_error(notify_api, sample_service):