diff --git a/apps/api/plane/api/serializers/issue.py b/apps/api/plane/api/serializers/issue.py index 5a771f2ad5c..e0e7d592767 100644 --- a/apps/api/plane/api/serializers/issue.py +++ b/apps/api/plane/api/serializers/issue.py @@ -105,18 +105,32 @@ def validate(self, data): # Validate assignees are from project if data.get("assignees", []): - data["assignees"] = ProjectMember.objects.filter( - project_id=self.context.get("project_id"), - is_active=True, - role__gte=15, - member_id__in=data["assignees"], - ).values_list("member_id", flat=True) + valid_assignee_ids = set( + ProjectMember.objects.filter( + project_id=self.context.get("project_id"), + is_active=True, + role__gte=15, + member_id__in=data["assignees"], + ).values_list("member_id", flat=True) + ) + invalid_assignee_ids = set(data["assignees"]) - valid_assignee_ids + if invalid_assignee_ids: + raise serializers.ValidationError( + f"Assignees {list(invalid_assignee_ids)} are not active members of this project" + ) + data["assignees"] = list(valid_assignee_ids) # Validate labels are from project if data.get("labels", []): - data["labels"] = Label.objects.filter( - project_id=self.context.get("project_id"), id__in=data["labels"] - ).values_list("id", flat=True) + valid_label_ids = set( + Label.objects.filter( + project_id=self.context.get("project_id"), id__in=data["labels"] + ).values_list("id", flat=True) + ) + invalid_label_ids = set(data["labels"]) - valid_label_ids + if invalid_label_ids: + raise serializers.ValidationError(f"Labels {list(invalid_label_ids)} do not belong to this project") + data["labels"] = list(valid_label_ids) # Check state is from the project only else raise validation error if ( diff --git a/apps/api/plane/tests/contract/api/test_issue_assignee_label_validation.py b/apps/api/plane/tests/contract/api/test_issue_assignee_label_validation.py new file mode 100644 index 00000000000..768a2b6e445 --- /dev/null +++ b/apps/api/plane/tests/contract/api/test_issue_assignee_label_validation.py @@ -0,0 +1,136 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +import pytest +from rest_framework import status + +from plane.celery import app as celery_app +from plane.db.models import Issue, Label, Project, ProjectMember, State, User + + +@pytest.fixture(autouse=True) +def celery_eager(): + """ + Run Celery tasks synchronously in-process instead of publishing to a + broker. There's no RabbitMQ/broker in this local sandbox, and these + tests only care about the HTTP response contract, not async delivery. + """ + original = celery_app.conf.task_always_eager + celery_app.conf.task_always_eager = True + celery_app.conf.task_eager_propagates = False + yield + celery_app.conf.task_always_eager = original + + +@pytest.fixture +def project(db, workspace, create_user): + """Create a test project with the user as an admin member and a default state.""" + project = Project.objects.create( + name="Test Project", identifier="TP", workspace=workspace, created_by=create_user + ) + ProjectMember.objects.create(project=project, member=create_user, role=20, is_active=True) + State.objects.create( + name="Backlog", + color="#000000", + group="backlog", + default=True, + project=project, + workspace=workspace, + created_by=create_user, + ) + return project + + +@pytest.fixture +def create_issue(db, project, workspace, create_user): + return Issue.objects.create(name="Existing Issue", project=project, workspace=workspace, created_by=create_user) + + +@pytest.fixture +def outsider_user(db): + """A user who exists in the workspace/system but is NOT a member of `project`.""" + user = User.objects.create(email="outsider@plane.so", username="outsider-user") + user.set_password("outsider-password") + user.save() + return user + + +@pytest.mark.contract +class TestIssueAssigneeLabelValidationContract: + """ + Contract: creating/updating a work item through the external REST API + (``/api/v1/...``) must reject assignee/label ids that don't belong to the + project with a 400, instead of silently dropping them and returning + 200/201. See makeplane/plane#9517. + """ + + def get_list_url(self, workspace_slug, project_id): + return f"/api/v1/workspaces/{workspace_slug}/projects/{project_id}/issues/" + + def get_detail_url(self, workspace_slug, project_id, issue_id): + return f"/api/v1/workspaces/{workspace_slug}/projects/{project_id}/issues/{issue_id}/" + + @pytest.mark.django_db + def test_create_with_non_member_assignee_is_rejected(self, api_key_client, workspace, project, outsider_user): + url = self.get_list_url(workspace.slug, project.id) + + response = api_key_client.post( + url, {"name": "New Issue", "assignees": [str(outsider_user.id)]}, format="json" + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert not Issue.objects.filter(name="New Issue").exists() + + @pytest.mark.django_db + def test_create_with_foreign_label_is_rejected(self, api_key_client, workspace, project): + other_project = Project.objects.create(name="Other", identifier="OTH", workspace=workspace) + foreign_label = Label.objects.create(name="Foreign", project=other_project) + + url = self.get_list_url(workspace.slug, project.id) + response = api_key_client.post( + url, {"name": "New Issue", "labels": [str(foreign_label.id)]}, format="json" + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert not Issue.objects.filter(name="New Issue").exists() + + @pytest.mark.django_db + def test_update_with_non_member_assignee_is_rejected_and_leaves_assignees_unchanged( + self, api_key_client, workspace, project, create_issue, outsider_user + ): + url = self.get_detail_url(workspace.slug, project.id, create_issue.id) + + response = api_key_client.patch(url, {"assignees": [str(outsider_user.id)]}, format="json") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert list(create_issue.assignees.all()) == [] + + @pytest.mark.django_db + def test_create_with_mix_of_valid_and_invalid_assignee_is_rejected_entirely( + self, api_key_client, workspace, project, create_user, outsider_user + ): + """A partially-valid list must reject the whole request, not silently keep only the valid id.""" + url = self.get_list_url(workspace.slug, project.id) + + response = api_key_client.post( + url, + {"name": "New Issue", "assignees": [str(create_user.id), str(outsider_user.id)]}, + format="json", + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert not Issue.objects.filter(name="New Issue").exists() + + @pytest.mark.django_db + def test_create_with_valid_assignee_still_works(self, api_key_client, workspace, project, create_user): + """Regression guard: a genuinely valid project member must still be assignable.""" + url = self.get_list_url(workspace.slug, project.id) + + response = api_key_client.post( + url, {"name": "New Issue", "assignees": [str(create_user.id)]}, format="json" + ) + + assert response.status_code == status.HTTP_201_CREATED + issue = Issue.objects.get(name="New Issue") + assert list(issue.assignees.values_list("id", flat=True)) == [create_user.id] diff --git a/apps/api/plane/tests/unit/serializers/test_issue_serializer_api.py b/apps/api/plane/tests/unit/serializers/test_issue_serializer_api.py new file mode 100644 index 00000000000..8b3c8ff75d7 --- /dev/null +++ b/apps/api/plane/tests/unit/serializers/test_issue_serializer_api.py @@ -0,0 +1,61 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +import pytest + +from plane.api.serializers.issue import IssueSerializer +from plane.db.models import Project, ProjectMember, Label, User + + +@pytest.mark.unit +class TestIssueSerializerAssigneeAndLabelValidation: + """Test that IssueSerializer rejects invalid assignees/labels instead of silently dropping them""" + + @pytest.mark.django_db + def test_rejects_assignee_who_is_not_an_active_project_member(self, db, workspace, create_user): + """An assignee id that isn't an active project member (role >= 15) must raise a validation error""" + project = Project.objects.create(name="Test Project", identifier="TEST", workspace=workspace) + + outsider = User.objects.create( + email="outsider@example.com", first_name="Out", last_name="Sider", username="outsider" + ) + # Not a member of the project at all + + serializer = IssueSerializer( + data={"name": "Test Issue", "assignees": [str(outsider.id)]}, + context={"project_id": project.id, "workspace_id": workspace.id}, + ) + + assert not serializer.is_valid() + assert "assignees" in str(serializer.errors).lower() or "assignee" in str(serializer.errors).lower() + + @pytest.mark.django_db + def test_rejects_label_that_does_not_belong_to_project(self, db, workspace, create_user): + """A label id that belongs to a different project must raise a validation error""" + project = Project.objects.create(name="Test Project", identifier="TEST", workspace=workspace) + other_project = Project.objects.create(name="Other Project", identifier="OTHER", workspace=workspace) + + foreign_label = Label.objects.create(name="Foreign Label", project=other_project) + + serializer = IssueSerializer( + data={"name": "Test Issue", "labels": [str(foreign_label.id)]}, + context={"project_id": project.id, "workspace_id": workspace.id}, + ) + + assert not serializer.is_valid() + assert "labels" in str(serializer.errors).lower() or "label" in str(serializer.errors).lower() + + @pytest.mark.django_db + def test_accepts_assignee_who_is_an_active_project_member(self, db, workspace, create_user): + """A valid active project member id should still be accepted""" + project = Project.objects.create(name="Test Project", identifier="TEST", workspace=workspace) + ProjectMember.objects.create(project=project, member=create_user, role=15, is_active=True) + + serializer = IssueSerializer( + data={"name": "Test Issue", "assignees": [str(create_user.id)]}, + context={"project_id": project.id, "workspace_id": workspace.id}, + ) + + assert serializer.is_valid(), serializer.errors + assert list(serializer.validated_data["assignees"]) == [create_user.id]