diff --git a/apps/api/plane/api/views/issue.py b/apps/api/plane/api/views/issue.py index fc81bc27c6..dd51bd81d6 100644 --- a/apps/api/plane/api/views/issue.py +++ b/apps/api/plane/api/views/issue.py @@ -2023,10 +2023,9 @@ class IssueAttachmentListCreateAPIEndpoint(BaseAPIView): List all attachments for an issue. """ - # This endpoint has no permission_classes beyond IsAuthenticated, and API - # tokens authenticate globally (not workspace-scoped), so enforce project - # membership on the issue the same way post() does — otherwise any token - # holder could read any issue's attachment metadata cross-tenant. + # Bare IsAuthenticated here, and API tokens authenticate globally rather + # than per workspace, so enforce project membership as post() does -- + # otherwise any token holder reads any issue's attachment metadata. issue = Issue.objects.get(pk=issue_id, workspace__slug=slug, project_id=project_id) if not user_has_issue_permission( request.user.id, @@ -2077,20 +2076,34 @@ class IssueAttachmentDetailAPIEndpoint(BaseAPIView): Records deletion activity and triggers metadata cleanup. """ issue = Issue.objects.get(pk=issue_id, workspace__slug=slug, project_id=project_id) - # if the request user is creator or admin then delete the attachment - if not user_has_issue_permission( + + # Bind the asset to the URL work item and to the attachment entity type, + # matching the sibling list handler. Resolved before authorizing, so the + # check below applies to the object actually being acted on. + issue_attachment = FileAsset.objects.get( + pk=pk, + workspace__slug=slug, + project_id=project_id, + issue_id=issue_id, + entity_type=FileAsset.EntityTypeContext.ISSUE_ATTACHMENT, + ) + + # Admin or the uploader, as the app surface allows. allow_creator is off + # because it compares against the work item's author, not this file's + # uploader, so the ownership test is done explicitly below. + is_project_admin = user_has_issue_permission( request.user.id, project_id=project_id, issue=issue, - allowed_roles=[ROLE.ADMIN.value, ROLE.MEMBER.value, ROLE.GUEST.value], - allow_creator=True, - ): + allowed_roles=[ROLE.ADMIN.value], + allow_creator=False, + ) + if not (is_project_admin or issue_attachment.created_by_id == request.user.id): return Response( {"error": "You are not allowed to delete this attachment"}, status=status.HTTP_403_FORBIDDEN, ) - issue_attachment = FileAsset.objects.get(pk=pk, workspace__slug=slug, project_id=project_id) issue_attachment.is_deleted = True issue_attachment.deleted_at = timezone.now() issue_attachment.save() @@ -2163,8 +2176,16 @@ class IssueAttachmentDetailAPIEndpoint(BaseAPIView): status=status.HTTP_403_FORBIDDEN, ) - # Get the asset - asset = FileAsset.objects.get(id=pk, workspace__slug=slug, project_id=project_id) + # Bind to the URL work item and entity type: scoped to the project alone, + # this route issued a presigned URL for any asset in it, page and comment + # images included. + asset = FileAsset.objects.get( + id=pk, + workspace__slug=slug, + project_id=project_id, + issue_id=issue_id, + entity_type=FileAsset.EntityTypeContext.ISSUE_ATTACHMENT, + ) # Check if the asset is uploaded if not asset.is_uploaded: @@ -2228,7 +2249,14 @@ class IssueAttachmentDetailAPIEndpoint(BaseAPIView): status=status.HTTP_403_FORBIDDEN, ) - issue_attachment = FileAsset.objects.get(pk=pk, workspace__slug=slug, project_id=project_id) + # Bound as in the delete and retrieve handlers above. + issue_attachment = FileAsset.objects.get( + pk=pk, + workspace__slug=slug, + project_id=project_id, + issue_id=issue_id, + entity_type=FileAsset.EntityTypeContext.ISSUE_ATTACHMENT, + ) serializer = IssueAttachmentSerializer(issue_attachment) # Send this activity only if the attachment is not uploaded before @@ -2245,9 +2273,10 @@ class IssueAttachmentDetailAPIEndpoint(BaseAPIView): origin=base_host(request=request, is_app=True), ) - # Update the attachment + # created_by is deliberately not reassigned: confirming an upload is + # not authorship, and rewriting it would hand the caller the + # creator-based delete right on someone else's attachment. issue_attachment.is_uploaded = True - issue_attachment.created_by = request.user # Get the storage metadata if not issue_attachment.storage_metadata: diff --git a/apps/api/plane/tests/contract/api/test_external_api_issue_authz.py b/apps/api/plane/tests/contract/api/test_external_api_issue_authz.py index a7bc54d6da..1053c6e127 100644 --- a/apps/api/plane/tests/contract/api/test_external_api_issue_authz.py +++ b/apps/api/plane/tests/contract/api/test_external_api_issue_authz.py @@ -182,3 +182,170 @@ class TestExternalApiAttachmentAuthz: f"Got {response.status_code}: {getattr(response, 'data', None)!r}" ) assert len(response.data) == 1 + + +# --- attachment object binding + ownership transfer --------------------------- + +ATTACH_DETAIL_URL = "/api/v1/workspaces/{slug}/projects/{project_id}/issues/{issue_id}/issue-attachments/{pk}/" + + +@pytest.fixture +def other_issue(db, workspace, project, create_user): + """A second work item in the same project, which the attachment does NOT belong to.""" + issue = Issue(name="Other Issue", project=project, workspace=workspace) + issue.save(created_by_id=create_user.id) + return issue + + +@pytest.fixture +def page_image(db, workspace, project, create_user): + """A page inline image: same project, no issue, a different entity type.""" + return FileAsset.objects.create( + attributes={"name": "diagram.png", "type": "image/png", "size": 100}, + asset=f"{workspace.id}/{uuid4().hex}-diagram.png", + size=100, + workspace=workspace, + project=project, + created_by=create_user, + entity_type=FileAsset.EntityTypeContext.PAGE_DESCRIPTION, + is_uploaded=True, + ) + + +@pytest.mark.contract +@pytest.mark.django_db +class TestExternalApiAttachmentObjectBinding: + """The permission is checked against the URL work item; the object must match it. + + Resolved by project alone, any asset in it came back -- page and comment + images included -- so naming a work item you may act on reached all of them. + """ + + def test_admin_cannot_delete_attachment_through_a_different_work_item( + self, workspace, project, issue, other_issue, attachment, create_user + ): + # Admin, so the 404 is attributable to the binding, not the role check. + response = _token_client(create_user).delete( + ATTACH_DETAIL_URL.format( + slug=workspace.slug, project_id=project.id, issue_id=other_issue.id, pk=attachment.id + ) + ) + assert response.status_code == status.HTTP_404_NOT_FOUND, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + attachment.refresh_from_db() + assert attachment.is_deleted is False, "an attachment of another work item was destroyed" + + def test_member_cannot_read_attachment_through_a_different_work_item( + self, workspace, project, issue, other_issue, attachment + ): + # Retrieve is open to any project member, so a member is the right caller. + member = _project_member(workspace, project, role=15) + response = _token_client(member).get( + ATTACH_DETAIL_URL.format( + slug=workspace.slug, project_id=project.id, issue_id=other_issue.id, pk=attachment.id + ) + ) + assert response.status_code == status.HTTP_404_NOT_FOUND, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + + def test_page_image_is_not_reachable_through_the_attachment_route( + self, workspace, project, issue, page_image, create_user + ): + """The cross-entity half: a page's inline image is not a work item attachment.""" + # Admin again, so the refusal is the entity_type binding and not the role. + response = _token_client(create_user).delete( + ATTACH_DETAIL_URL.format( + slug=workspace.slug, project_id=project.id, issue_id=issue.id, pk=page_image.id + ) + ) + assert response.status_code == status.HTTP_404_NOT_FOUND, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + page_image.refresh_from_db() + assert page_image.is_deleted is False, "a page image was destroyed through the attachment route" + + def test_admin_can_delete_a_correctly_bound_attachment( + self, workspace, project, issue, attachment, create_user + ): + """Positive control: the binding must not break the legitimate path.""" + response = _token_client(create_user).delete( + ATTACH_DETAIL_URL.format( + slug=workspace.slug, project_id=project.id, issue_id=issue.id, pk=attachment.id + ) + ) + assert response.status_code == status.HTTP_204_NO_CONTENT, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + attachment.refresh_from_db() + assert attachment.is_deleted is True + + +@pytest.mark.contract +@pytest.mark.django_db +class TestExternalApiAttachmentDeleteRole: + """Delete is admin-or-uploader, matching the app surface. + + Listing every role made the filter a no-op, reducing it to any active project + member while the handler's own comment claimed "creator or admin". + """ + + def test_member_who_did_not_upload_cannot_delete(self, workspace, project, issue, attachment): + member = _project_member(workspace, project, role=15) + response = _token_client(member).delete( + ATTACH_DETAIL_URL.format( + slug=workspace.slug, project_id=project.id, issue_id=issue.id, pk=attachment.id + ) + ) + assert response.status_code == status.HTTP_403_FORBIDDEN, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + attachment.refresh_from_db() + assert attachment.is_deleted is False + + def test_uploader_can_delete_own_attachment(self, workspace, project, issue, attachment): + uploader = _project_member(workspace, project, role=15) + # A queryset update, not save(): BaseModel.save() nulls created_by when no + # request is in scope, which is the case in a test. + FileAsset.objects.filter(pk=attachment.id).update(created_by=uploader) + + response = _token_client(uploader).delete( + ATTACH_DETAIL_URL.format( + slug=workspace.slug, project_id=project.id, issue_id=issue.id, pk=attachment.id + ) + ) + assert response.status_code == status.HTTP_204_NO_CONTENT, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + + +@pytest.mark.contract +@pytest.mark.django_db +class TestExternalApiAttachmentOwnershipTransfer: + """Confirming an upload must not rewrite created_by. + + Whoever confirmed became the recorded uploader, and delete grants the creator + a delete right -- so the reassignment handed over someone else's attachment. + """ + + def test_patch_does_not_transfer_ownership(self, workspace, project, issue, attachment, create_user): + # See above: set ownership through the queryset so save() cannot clear it. + FileAsset.objects.filter(pk=attachment.id).update(is_uploaded=False, created_by=create_user) + attachment.refresh_from_db() + + member = _project_member(workspace, project, role=15) + response = _token_client(member).patch( + ATTACH_DETAIL_URL.format( + slug=workspace.slug, project_id=project.id, issue_id=issue.id, pk=attachment.id + ), + {"is_uploaded": True}, + format="json", + ) + + assert response.status_code == status.HTTP_204_NO_CONTENT, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + attachment.refresh_from_db() + assert attachment.is_uploaded is True, "the upload confirmation itself must still work" + assert attachment.created_by_id == create_user.id, "created_by was transferred to the caller"