Test and fix empty action handling (#413)

### Requirements List
- Bug fix for #412 

### Description List
- Revised empty action handling in the staff-user permissions api

Closes #
#412

Author: jusdino

backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/base_record.py

@@ -35,6 +35,8 @@ def __init__(self, *args, **kwargs):
 class Set(List):
     """A Field that de/serializes to a Set (not compatible with JSON)"""
 
+    default_error_messages = {'invalid': 'Not a valid set.'}
+
     def _serialize(self, *args, **kwargs):
         return set(super()._serialize(*args, **kwargs))
 

backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/user.py

@@ -18,6 +18,19 @@ class CompactPermissionsRecordSchema(Schema):
         allow_none=False,
     )
 
+    @post_dump
+    def drop_empty_actions(self, data, **kwargs):  # noqa: ARG002 unused-kwargs
+        """
+        DynamoDB doesn't like empty sets, so we will make a point to drop an actions field entirely,
+        if it is empty.
+        """
+        if not data.get('actions', {}):
+            data.pop('actions', None)
+        empty_jurisdictions = [jurisdiction for jurisdiction, actions in data['jurisdictions'].items() if not actions]
+        for jurisdiction in empty_jurisdictions:
+            del data['jurisdictions'][jurisdiction]
+        return data
+
 
 class UserAttributesSchema(Schema):
     email = String(required=True, allow_none=False, validate=Length(1, 100))
@@ -63,7 +76,7 @@ class CompactActionPermissionAPISchema(Schema):
     actions = Dict(
         keys=String(),  # Keys are actions
         values=Boolean(),
-        required=True,
+        required=False,
         allow_none=False,
     )
 
@@ -90,7 +103,7 @@ class UserAPISchema(Schema):
     dateOfUpdate = Raw(required=True, allow_none=False)
     attributes = Nested(UserAttributesSchema(), required=True, allow_none=False)
     permissions = Dict(
-        keys=String(validate=OneOf(config.compacts)),
+        keys=String(validate=OneOf(config.compacts)),  # Key is one compact
         values=Nested(CompactPermissionsAPISchema(), required=True, allow_none=False),
         validate=Length(equal=1),
     )
@@ -128,12 +141,14 @@ def transform_to_dynamo_permissions(self, data, **kwargs):  # noqa: ARG002 unuse
             data['compact'] = compact
 
         # { "actions": { "read": True } } -> { "actions": { "read" } }
-        data['permissions']['actions'] = {key for key, value in data['permissions']['actions'].items() if value is True}
+        data['permissions']['actions'] = {
+            key for key, value in data['permissions'].get('actions', {}).items() if value is True
+        }
 
         # { "oh": { "actions": { "write": True } } } -> { "oh": { "write" } }
         for jurisdiction, jurisdiction_permissions in data['permissions']['jurisdictions'].items():
             data['permissions']['jurisdictions'][jurisdiction] = {
-                key for key, value in jurisdiction_permissions['actions'].items() if value is True
+                key for key, value in jurisdiction_permissions.get('actions', {}).items() if value is True
             }
 
         return data

backend/compact-connect/lambdas/python/staff-users/tests/function/test_handlers/test_patch_user.py

@@ -105,6 +105,79 @@ def test_patch_user_document_path_overlap(self):
             user,
         )
 
+    def test_patch_user_add_to_empty_actions(self):
+        from handlers.users import patch_user, post_user
+
+        with open('tests/resources/api-event.json') as f:
+            event = json.load(f)
+
+        with open('tests/resources/api/user-post.json') as f:
+            api_user = json.load(f)
+        # Create a user with no compact read or admin, no actions in a jurisdiction
+        api_user['permissions'] = {'aslp': {'jurisdictions': {}}}
+        event['body'] = json.dumps(api_user)
+
+        event['requestContext']['authorizer']['claims']['scope'] = 'openid email aslp/admin aslp/aslp.admin'
+        event['pathParameters'] = {'compact': 'aslp'}
+
+        resp = post_user(event, self.mock_context)
+        self.assertEqual(200, resp['statusCode'])
+        user = json.loads(resp['body'])
+        user_id = user.pop('userId')
+
+        # Add compact read and oh admin permissions to the user
+        event['pathParameters'] = {'compact': 'aslp', 'userId': user_id}
+        api_user['permissions'] = {
+            'aslp': {'actions': {'read': True}, 'jurisdictions': {'oh': {'actions': {'admin': True}}}}
+        }
+        event['body'] = json.dumps(api_user)
+
+        resp = patch_user(event, self.mock_context)
+        self.assertEqual(200, resp['statusCode'])
+        user = json.loads(resp['body'])
+
+        # Drop backend-generated fields from comparison
+        del user['userId']
+        del user['dateOfUpdate']
+
+        self.assertEqual(api_user, user)
+
+    def test_patch_user_remove_all_actions(self):
+        from handlers.users import patch_user, post_user
+
+        with open('tests/resources/api-event.json') as f:
+            event = json.load(f)
+
+        with open('tests/resources/api/user-post.json') as f:
+            api_user = json.load(f)
+
+        event['requestContext']['authorizer']['claims']['scope'] = 'openid email aslp/admin aslp/aslp.admin'
+        event['pathParameters'] = {'compact': 'aslp'}
+        event['body'] = json.dumps(api_user)
+
+        resp = post_user(event, self.mock_context)
+        self.assertEqual(200, resp['statusCode'])
+        user = json.loads(resp['body'])
+        user_id = user.pop('userId')
+
+        # Remove all the permissions from the user
+        event['pathParameters'] = {'compact': 'aslp', 'userId': user_id}
+        api_user['permissions'] = {
+            'aslp': {'actions': {'read': False}, 'jurisdictions': {'oh': {'actions': {'write': False}}}}
+        }
+        event['body'] = json.dumps(api_user)
+
+        resp = patch_user(event, self.mock_context)
+        self.assertEqual(200, resp['statusCode'])
+        user = json.loads(resp['body'])
+
+        # Drop backend-generated fields from comparison
+        del user['userId']
+        del user['dateOfUpdate']
+
+        api_user['permissions'] = {'aslp': {'jurisdictions': {}}}
+        self.assertEqual(api_user, user)
+
     def test_patch_user_forbidden(self):
         self._load_user_data()
 

backend/compact-connect/lambdas/python/staff-users/tests/function/test_handlers/test_post_user.py

@@ -34,6 +34,49 @@ def test_post_user(self):
 
         self.assertEqual(api_user, user)
 
+    def test_post_user_no_compact_perms_round_trip(self):
+        from handlers.users import get_one_user, post_user
+
+        with open('tests/resources/api-event.json') as f:
+            event = json.load(f)
+
+        with open('tests/resources/api/user-post.json') as f:
+            api_user = json.load(f)
+        # A user with no compact read or admin, no actions in a jurisdiction
+        api_user['permissions'] = {'aslp': {'actions': {}, 'jurisdictions': {'oh': {'actions': {}}}}}
+        event['body'] = json.dumps(api_user)
+
+        # The user has admin permission for aslp admin
+        event['requestContext']['authorizer']['claims']['scope'] = (
+            'openid email aslp/admin aslp/aslp.admin aslp/oh.admin'
+        )
+        event['pathParameters'] = {'compact': 'aslp'}
+
+        resp = post_user(event, self.mock_context)
+        self.assertEqual(200, resp['statusCode'])
+        user = json.loads(resp['body'])
+
+        # Drop backend-generated fields from comparison
+        user_id = user.pop('userId')
+        del user['dateOfUpdate']
+        # The aslp.actions and aslp.jurisdictions.oh fields should be removed, since they are empty
+        api_user['permissions'] = {'aslp': {'jurisdictions': {}}}
+
+        self.assertEqual(api_user, user)
+
+        # Get the user back out via the API to check GET vs POST consistency
+        del event['body']
+        event['pathParameters'] = {'compact': 'aslp', 'userId': user_id}
+        resp = get_one_user(event, self.mock_context)
+        self.assertEqual(200, resp['statusCode'])
+        user = json.loads(resp['body'])
+
+        # Drop backend-generated fields from comparison
+        del user['userId']
+        del user['dateOfUpdate']
+
+        self.assertEqual(api_user, user)
+
     def test_post_user_unauthorized(self):
         from handlers.users import post_user
 

backend/compact-connect/stacks/api_stack/v1_api/api.py

@@ -124,4 +124,5 @@ def __init__(self, root: IResource, persistent_stack: ps.PersistentStack):
             self_resource=staff_users_self_resource,
             admin_scopes=admin_scopes,
             persistent_stack=persistent_stack,
+            api_model=self.api_model,
         )

backend/compact-connect/stacks/api_stack/v1_api/api_model.py

@@ -171,6 +171,166 @@ def bulk_upload_response_model(self) -> Model:
         )
         return self.api.bulk_upload_response_model
 
+    @property
+    def post_staff_user_model(self):
+        """Return the Post User Model, which should only be created once per API"""
+        if hasattr(self.api, 'v1_post_user_request_model'):
+            return self.api.v1_post_user_request_model
+
+        self.api.v1_post_user_request_model = self.api.add_model(
+            'V1PostUserRequestModel',
+            description='Post user request model',
+            schema=JsonSchema(
+                type=JsonSchemaType.OBJECT,
+                required=['attributes', 'permissions'],
+                additional_properties=False,
+                properties=self._common_staff_user_properties,
+            ),
+        )
+        return self.api.v1_post_user_request_model
+
+    @property
+    def get_staff_user_me_model(self):
+        """Return the Get Me Model, which should only be created once per API"""
+        if hasattr(self.api, 'v1_get_me_model'):
+            return self.api.v1_get_me_model
+
+        self.api.v1_get_me_model = self.api.add_model(
+            'V1GetMeModel',
+            description='Get me response model',
+            schema=self._staff_user_response_schema,
+        )
+        return self.api.v1_get_me_model
+
+    @property
+    def get_staff_users_response_model(self):
+        """Return the Get Users Model, which should only be created once per API"""
+        if hasattr(self.api, 'v1_get_staff_users_response_model'):
+            return self.api.v1_get_staff_users_response_model
+
+        self.api.v1_get_staff_users_response_model = self.api.add_model(
+            'V1GetStaffUsersModel',
+            description='Get staff users response model',
+            schema=JsonSchema(
+                type=JsonSchemaType.OBJECT,
+                additional_properties=False,
+                properties={
+                    'users': JsonSchema(type=JsonSchemaType.ARRAY, items=self._staff_user_response_schema),
+                    'pagination': self._pagination_response_schema,
+                },
+            ),
+        )
+        return self.api.v1_get_staff_users_response_model
+
+    @property
+    def patch_staff_user_me_model(self):
+        """Return the Get Me Model, which should only be created once per API"""
+        if hasattr(self.api, 'v1_patch_me_request_model'):
+            return self.api.v1_patch_me_request_model
+
+        self.api.v1_patch_me_request_model = self.api.add_model(
+            'V1PatchMeRequestModel',
+            description='Patch me request model',
+            schema=JsonSchema(
+                type=JsonSchemaType.OBJECT,
+                additional_properties=False,
+                properties={
+                    'attributes': self._staff_user_patch_attributes_schema,
+                },
+            ),
+        )
+        return self.api.v1_patch_me_request_model
+
+    @property
+    def patch_staff_user_model(self):
+        """Return the Patch User Model, which should only be created once per API"""
+        if hasattr(self.api, 'v1_patch_user_request_model'):
+            return self.api.v1_patch_user_request_model
+
+        self.api.v1_patch_user_request_model = self.api.add_model(
+            'V1PatchUserRequestModel',
+            description='Patch user request model',
+            schema=JsonSchema(
+                type=JsonSchemaType.OBJECT,
+                additional_properties=False,
+                properties={'permissions': self._staff_user_permissions_schema},
+            ),
+        )
+        return self.api.v1_patch_user_request_model
+
+    @property
+    def _staff_user_attributes_schema(self):
+        return JsonSchema(
+            type=JsonSchemaType.OBJECT,
+            required=['email', 'givenName', 'familyName'],
+            additional_properties=False,
+            properties={
+                'email': JsonSchema(type=JsonSchemaType.STRING, min_length=5, max_length=100),
+                'givenName': JsonSchema(type=JsonSchemaType.STRING, min_length=1, max_length=100),
+                'familyName': JsonSchema(type=JsonSchemaType.STRING, min_length=1, max_length=100),
+            },
+        )
+
+    @property
+    def _staff_user_patch_attributes_schema(self):
+        """No support for changing a user's email address."""
+        return JsonSchema(
+            type=JsonSchemaType.OBJECT,
+            additional_properties=False,
+            properties={
+                'givenName': JsonSchema(type=JsonSchemaType.STRING, min_length=1, max_length=100),
+                'familyName': JsonSchema(type=JsonSchemaType.STRING, min_length=1, max_length=100),
+            },
+        )
+
+    @property
+    def _staff_user_permissions_schema(self):
+        return JsonSchema(
+            type=JsonSchemaType.OBJECT,
+            additional_properties=JsonSchema(
+                type=JsonSchemaType.OBJECT,
+                additional_properties=False,
+                properties={
+                    'actions': JsonSchema(
+                        type=JsonSchemaType.OBJECT,
+                        properties={
+                            'read': JsonSchema(type=JsonSchemaType.BOOLEAN),
+                            'admin': JsonSchema(type=JsonSchemaType.BOOLEAN),
+                        },
+                    ),
+                    'jurisdictions': JsonSchema(
+                        type=JsonSchemaType.OBJECT,
+                        additional_properties=JsonSchema(
+                            type=JsonSchemaType.OBJECT,
+                            properties={
+                                'actions': JsonSchema(
+                                    type=JsonSchemaType.OBJECT,
+                                    additional_properties=False,
+                                    properties={
+                                        'write': JsonSchema(type=JsonSchemaType.BOOLEAN),
+                                        'admin': JsonSchema(type=JsonSchemaType.BOOLEAN),
+                                    },
+                                ),
+                            },
+                        ),
+                    ),
+                },
+            ),
+        )
+
+    @property
+    def _common_staff_user_properties(self):
+        return {'attributes': self._staff_user_attributes_schema, 'permissions': self._staff_user_permissions_schema}
+
+    @property
+    def _staff_user_response_schema(self):
+        return JsonSchema(
+            type=JsonSchemaType.OBJECT,
+            required=['userId', 'attributes', 'permissions'],
+            additional_properties=False,
+            properties={'userId': JsonSchema(type=JsonSchemaType.STRING), **self._common_staff_user_properties},
+        )
+
     @property
     def post_provider_user_military_affiliation_request_model(self) -> Model:
         """Return the post payment processor credentials request model, which should only be created once per API"""