Merge pull request #5689 from FinnStutzenstein/fixManageSpeakerValidation

Add user_id validation for LOS and assignment candidates

server/openslides/agenda/views.py

@@ -346,6 +346,9 @@ class ListOfSpeakersViewSet(
                     raise ValidationError({"detail": "The list of speakers is closed."})
                 user = self.request.user
             else:
+                if not isinstance(user_id, int):
+                    raise ValidationError({"detail": "user_id has to be an int."})
+
                 point_of_order = False  # not for someone else
                 # Add someone else.
                 if not has_perm(
@@ -353,8 +356,8 @@ class ListOfSpeakersViewSet(
                 ):
                     self.permission_denied(request)
                 try:
-                    user = get_user_model().objects.get(pk=int(user_id))
+                    user = get_user_model().objects.get(pk=user_id)
-                except (ValueError, get_user_model().DoesNotExist):
+                except get_user_model().DoesNotExist:
                     raise ValidationError({"detail": "User does not exist."})
 
             # Try to add the user. This ensurse that a user is not twice in the

server/openslides/assignments/views.py

@@ -126,46 +126,29 @@ class AssignmentViewSet(ModelViewSet):
         assignment.remove_candidate(request.user)
         return "You have withdrawn your candidature successfully."
 
-    def get_user_from_request_data(self, request):
-        """
-        Helper method to get a specific user from request data (not the
-        request.user) so that the view self.candidature_other can play with it.
-        """
-        if not isinstance(request.data, dict):
-            raise ValidationError(
-                {
-                    "detail": "Invalid data. Expected dictionary, got {0}.",
-                    "args": [type(request.data)],
-                }
-            )
-        user_str = request.data.get("user", "")
-        try:
-            user_pk = int(user_str)
-        except ValueError:
-            raise ValidationError(
-                {"detail": 'Invalid data. Expected something like {"user": <id>}.'}
-            )
-        try:
-            user = get_user_model().objects.get(pk=user_pk)
-        except get_user_model().DoesNotExist:
-            raise ValidationError(
-                {"detail": "Invalid data. User {0} does not exist.", "args": [user_pk]}
-            )
-        return user
-
     @detail_route(methods=["post", "delete"])
     def candidature_other(self, request, pk=None):
         """
         View to nominate other users (POST) or delete their candidature
         status (DELETE). The client has to send {'user': <id>}.
         """
-        user = self.get_user_from_request_data(request)
+        user_id = request.data.get("user")
+        if not isinstance(user_id, int):
+            raise ValidationError({"detail": "user_id must be an int."})
+
+        try:
+            user = get_user_model().objects.get(pk=user_id)
+        except get_user_model().DoesNotExist:
+            raise ValidationError(
+                {"detail": "Invalid data. User {0} does not exist.", "args": [user_id]}
+            )
+
         assignment = self.get_object()
         if request.method == "POST":
             return self.nominate_other(request, user, assignment)
         else:
             # request.method == 'DELETE'
-            return self.delete_other(request, user, assignment)
+            return self.withdraw_other(request, user, assignment)
 
     def nominate_other(self, request, user, assignment):
         if assignment.phase == assignment.PHASE_FINISHED:
@@ -191,7 +174,7 @@ class AssignmentViewSet(ModelViewSet):
             {"detail": "User {0} was nominated successfully.", "args": [str(user)]}
         )
 
-    def delete_other(self, request, user, assignment):
+    def withdraw_other(self, request, user, assignment):
         # To delete candidature status you have to be a manager.
         if not has_perm(request.user, "assignments.can_manage"):
             self.permission_denied(request)

server/tests/integration/agenda/test_viewset.py

@@ -356,6 +356,13 @@ class ManageSpeaker(TestCase):
             ).exists()
         )
 
+    def test_add_someone_else_no_id(self):
+        response = self.client.post(
+            reverse("listofspeakers-manage-speaker", args=[self.list_of_speakers.pk]),
+            {"user": ["not valid"]},
+        )
+        self.assertEqual(response.status_code, 400)
+
     def test_point_of_order_single(self):
         config["agenda_enable_point_of_order_speakers"] = True
         self.assertEqual(Speaker.objects.all().count(), 0)

server/tests/integration/assignments/test_viewset.py

@@ -279,6 +279,14 @@ class CandidatureOther(TestCase):
             .exists()
         )
 
+    def test_nominate_other_invalid_id(self):
+        response = self.client.post(
+            reverse("assignment-candidature-other", args=[self.assignment.pk]),
+            {"user": ["not valid"]},
+        )
+
+        self.assertEqual(response.status_code, 400)
+
     def test_nominate_other_twice(self):
         self.assignment.add_candidate(
             get_user_model().objects.get(username="test_user_eeheekai4Phue6cahtho")

server/tests/unit/agenda/test_views.py

@@ -46,7 +46,7 @@ class ListOfSpeakersViewSetManageSpeaker(TestCase):
         self.request.method = "POST"
         self.request.user = 1
         self.request.data = {
-            "user": "2"
+            "user": 2
         }  # It is assumed that the request user has pk!=2.
         mock_get_user_model.return_value = MockUser = MagicMock()
         MockUser.objects.get.return_value = mock_user = MagicMock()