When adding a user new with permissions to a service, the permissions

dao was deleting all permissions for that user (regardless of service
id) as the last filter on the permissions dao get_query method won.

I've added a replace flag to the set_user_service_permission method
so that it can handle adding new users + permissions and editing
of existing users' permissions.

Also by pass the get_query method until it can be refactored to work
correctly.

For now execute the filter query directly on the model.

app/dao/permissions_dao.py

@@ -32,6 +32,8 @@ class PermissionDAO(DAOClass):
     class Meta:
         model = Permission
 
+    # TODO rework this as last filter wins, whereas what is needed is
+    # append to filter so that semantics are 'and'
     def get_query(self, filter_by_dict=None):
         if filter_by_dict is None:
             filter_by_dict = MultiDict()
@@ -60,13 +62,14 @@ class PermissionDAO(DAOClass):
             self.create_instance(permission, _commit=False)
 
     def remove_user_service_permissions(self, user, service):
-        query = self.get_query(filter_by_dict={'user': user.id, 'service': service.id})
+        query = self.Meta.model.query.filter_by(user=user, service=service)
         query.delete()
 
-    def set_user_service_permission(self, user, service, permissions, _commit=False):
+    def set_user_service_permission(self, user, service, permissions, _commit=False, replace=False):
         try:
-            query = self.get_query(filter_by_dict={'user': user.id, 'service': service.id})
-            query.delete()
+            if replace:
+                query = self.Meta.model.query.filter_by(user=user, service=service)
+                query.delete()
             for p in permissions:
                 p.user = user
                 p.service = service

app/service/rest.py

@@ -1,9 +1,13 @@
-from datetime import (datetime, date)
+from datetime import (
+    datetime,
+    date
+)
 
-from flask import Blueprint
 from flask import (
     jsonify,
-    request
+    request,
+    abort,
+    Blueprint
 )
 from sqlalchemy.orm.exc import NoResultFound
 
@@ -152,6 +156,8 @@ def add_user_to_service(service_id, user_id):
                        message='User id: {} already part of service id: {}'.format(user_id, service_id)), 400
 
     permissions, errors = permission_schema.load(request.get_json(), many=True)
+    if errors:
+        abort(400, errors)
 
     dao_add_user_to_service(service, user, permissions)
     data, errors = service_schema.dump(service)
@@ -174,15 +180,6 @@ def remove_user_from_service(service_id, user_id):
     return jsonify({}), 204
 
 
-def _process_permissions(user, service, permission_groups):
-    from app.permissions_utils import get_permissions_by_group
-    permissions = get_permissions_by_group(permission_groups)
-    for permission in permissions:
-        permission.user = user
-        permission.service = service
-    return permissions
-
-
 @service.route('/<uuid:service_id>/fragment/aggregate_statistics')
 def get_service_provider_aggregate_statistics(service_id):
     service = dao_fetch_service_by_id(service_id)

app/user/rest.py

@@ -188,7 +188,7 @@ def set_permissions(user_id, service_id):
     for p in permissions:
         p.user = user
         p.service = service
-    permission_dao.set_user_service_permission(user, service, permissions, _commit=True)
+    permission_dao.set_user_service_permission(user, service, permissions, _commit=True, replace=True)
     return jsonify({}), 204
 
 

tests/app/dao/test_services_dao.py

@@ -334,3 +334,53 @@ def test_delete_service_and_associated_objects(notify_db,
     assert InvitedUser.query.count() == 0
     assert Service.query.count() == 0
     assert Service.get_history_model().query.count() == 0
+
+
+def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sample_user):
+
+    service_one = Service(name="service_one",
+                          email_from="service_one",
+                          message_limit=1000,
+                          active=True,
+                          restricted=False,
+                          created_by=sample_user)
+
+    dao_create_service(service_one, sample_user)
+    assert sample_user.id == service_one.users[0].id
+    test_user_permissions = Permission.query.filter_by(service=service_one, user=sample_user).all()
+    assert len(test_user_permissions) == 8
+
+    other_user = User(
+        name='Other Test User',
+        email_address='other_user@digital.cabinet-office.gov.uk',
+        password='password',
+        mobile_number='+447700900987'
+    )
+    save_model_user(other_user)
+    service_two = Service(name="service_two",
+                          email_from="service_two",
+                          message_limit=1000,
+                          active=True,
+                          restricted=False,
+                          created_by=other_user)
+    dao_create_service(service_two, other_user)
+
+    assert other_user.id == service_two.users[0].id
+    other_user_permissions = Permission.query.filter_by(service=service_two, user=other_user).all()
+    assert len(other_user_permissions) == 8
+
+    other_user_service_one_permissions = Permission.query.filter_by(service=service_one, user=other_user).all()
+    assert len(other_user_service_one_permissions) == 0
+
+    # adding the other_user to service_one should leave all other_user permissions on service_two intact
+    permissions = []
+    for p in ['send_emails', 'send_texts', 'send_letters']:
+        permissions.append(Permission(permission=p))
+
+    dao_add_user_to_service(service_one, other_user, permissions=permissions)
+
+    other_user_service_one_permissions = Permission.query.filter_by(service=service_one, user=other_user).all()
+    assert len(other_user_service_one_permissions) == 3
+
+    other_user_service_two_permissions = Permission.query.filter_by(service=service_two, user=other_user).all()
+    assert len(other_user_service_two_permissions) == 8