From cd45df2f96fecb3805e414c3fb3d72b4af95f3d8 Mon Sep 17 00:00:00 2001
From: John Tordoff <>
Date: Fri, 6 Dec 2024 16:54:20 -0500
Subject: [PATCH] clean-up serializer/permission code to validate better, 409s
 instead of 403s etc

---
 api/users/permissions.py                      | 40 ++++---------------
 api/users/serializers.py                      |  9 +++--
 .../test_user_message_institutional_access.py |  6 +--
 3 files changed, 16 insertions(+), 39 deletions(-)

diff --git a/api/users/permissions.py b/api/users/permissions.py
index 688cfa236a1..7ca5e3ef862 100644
--- a/api/users/permissions.py
+++ b/api/users/permissions.py
@@ -68,41 +68,17 @@ def has_permission(self, request, view) -> bool:
         if not user or user.is_anonymous:
             return False
 
-        message_type = request.data.get('message_type')
-        if message_type == MessageTypes.INSTITUTIONAL_REQUEST:
-            return self._validate_institutional_request(request, user)
-
-        return False
-
-    def _validate_institutional_request(self, request, user: OSFUser) -> bool:
-        """
-        Validate the user's permissions for creating an `INSTITUTIONAL_REQUEST` message.
-        Args:
-            request: The HTTP request containing the institution ID.
-            user: The user making the request.
-        Returns:
-            bool: True if the user has the required permission.
-        """
         institution_id = request.data.get('institution')
         if not institution_id:
-            raise exceptions.ValidationError({'institution': 'Institution ID is required.'})
-
-        institution = self._get_institution(institution_id)
-
-        if not user.is_institutional_admin(institution):
-            raise exceptions.PermissionDenied('You are not an admin of the specified institution.')
+            raise exceptions.ValidationError({'institution': 'Institution is required.'})
 
-        return True
-
-    def _get_institution(self, institution_id: str) -> Institution:
-        """
-        Retrieve the institution by its ID.
-        Args:
-            institution_id (str): The ID of the institution.
-        Returns:
-            Institution: The retrieved institution.
-        """
         try:
-            return Institution.objects.get(_id=institution_id)
+            institution = Institution.objects.get(_id=institution_id)
         except Institution.DoesNotExist:
             raise exceptions.ValidationError({'institution': 'Specified institution does not exist.'})
+
+        message_type = request.data.get('message_type')
+        if message_type == MessageTypes.INSTITUTIONAL_REQUEST:
+            return user.is_institutional_admin(institution)
+        else:
+            raise exceptions.ValidationError('Not valid message type.')
diff --git a/api/users/serializers.py b/api/users/serializers.py
index 82cc37f31c4..6f2cfc7e1cd 100644
--- a/api/users/serializers.py
+++ b/api/users/serializers.py
@@ -707,9 +707,10 @@ def get_absolute_url(self, obj: UserMessage) -> str:
         )
 
     def to_internal_value(self, data):
-        instituion_id = data.pop('institution')
+        instituion_id = data.pop('institution', None)
         data = super().to_internal_value(data)
-        data['institution'] = instituion_id
+        if instituion_id:
+            data['institution'] = instituion_id
         return data
 
     class Meta:
@@ -751,10 +752,10 @@ def create(self, validated_data: Dict[str, Any]) -> UserMessage:
         )
 
         if not sender.is_institutional_admin(institution):
-            raise exceptions.ValidationError({'sender': 'Only institutional administrators can create messages.'})
+            raise Conflict({'sender': 'Only institutional administrators can create messages.'})
 
         if not recipient.is_affiliated_with_institution(institution):
-            raise exceptions.ValidationError(
+            raise Conflict(
                 {'user': 'Can not send to recipient that is not affiliated with the provided institution.'},
             )
 
diff --git a/api_tests/users/views/test_user_message_institutional_access.py b/api_tests/users/views/test_user_message_institutional_access.py
index a048b552c92..2da69424f76 100644
--- a/api_tests/users/views/test_user_message_institutional_access.py
+++ b/api_tests/users/views/test_user_message_institutional_access.py
@@ -116,7 +116,7 @@ def test_request_without_institution(self, app, institutional_admin, user, url_w
                 'source': {
                     'pointer': '/data/relationships/institution'
                 },
-                'detail': 'Institution ID is required.'
+                'detail': 'Institution is required.'
             }
         ]
 
@@ -150,6 +150,6 @@ def test_admin_cannot_message_user_outside_institution(
         Ensure that an institutional admin cannot create a `UserMessage` for a user who is not affiliated with their institution.
         """
         res = app.post_json_api(url_without_affiliation, payload, auth=institutional_admin.auth, expect_errors=True)
-        assert res.status_code == 400
+        assert res.status_code == 409
         assert 'Can not send to recipient that is not affiliated with the provided institution.'\
-               in res.json['errors'][0]['detail']
+               in res.json['errors'][0]['detail']['user']