mirror of
https://github.com/makeplane/plane.git
synced 2026-09-01 19:48:42 +02:00
[WEB-8332] fix: validate role before guest-cascade, make it atomic (Copilot #9460)
partial_update cascaded the guest project-role downgrade BEFORE the serializer was validated/saved, using a manual int() cast that 500s on non-integer input. Reorder: validate the serializer first, then cascade using serializer.validated_data["role"] inside a transaction.atomic() with save(), so a bad role returns 400 (not 500) and project roles can't be downgraded when the member update doesn't persist. Adds a non-integer-role 400 test. Co-authored-by: Plane AI <noreply@plane.so>
This commit is contained in:
@@ -3,6 +3,7 @@
|
||||
# See the LICENSE file for details.
|
||||
|
||||
# Django imports
|
||||
from django.db import transaction
|
||||
from django.db.models import Count, Q, OuterRef, Subquery, IntegerField
|
||||
from django.utils import timezone
|
||||
from django.db.models.functions import Coalesce
|
||||
@@ -84,10 +85,6 @@ class WorkSpaceMemberViewSet(BaseViewSet):
|
||||
status=status.HTTP_400_BAD_REQUEST,
|
||||
)
|
||||
|
||||
# If a user is moved to a guest role he can't have any other role in projects
|
||||
if "role" in request.data and int(request.data.get("role")) == 5:
|
||||
ProjectMember.objects.filter(workspace__slug=slug, member_id=workspace_member.member_id).update(role=5)
|
||||
|
||||
# SECURITY: The only field this endpoint is allowed to mutate is ``role``.
|
||||
# ``WorkSpaceMemberSerializer`` is declared with ``fields = "__all__"`` and
|
||||
# ``DynamicBaseSerializer`` ignores the ``fields=`` kwarg for writes, so
|
||||
@@ -101,10 +98,22 @@ class WorkSpaceMemberViewSet(BaseViewSet):
|
||||
|
||||
serializer = WorkSpaceMemberSerializer(workspace_member, data=allowed_data, partial=True)
|
||||
|
||||
if serializer.is_valid():
|
||||
if not serializer.is_valid():
|
||||
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
|
||||
|
||||
# Validate the role (via the serializer) BEFORE cascading, and do the
|
||||
# cascade + save atomically. Otherwise a bad role (e.g. non-integer) would
|
||||
# 500 on the manual int() cast, and the guest project-role downgrade could
|
||||
# persist even if the member update never succeeds.
|
||||
with transaction.atomic():
|
||||
# If a user is moved to a guest role they can't hold any other project role.
|
||||
if serializer.validated_data.get("role") == 5:
|
||||
ProjectMember.objects.filter(
|
||||
workspace__slug=slug, member_id=workspace_member.member_id
|
||||
).update(role=5)
|
||||
serializer.save()
|
||||
return Response(serializer.data, status=status.HTTP_200_OK)
|
||||
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
|
||||
|
||||
return Response(serializer.data, status=status.HTTP_200_OK)
|
||||
|
||||
@allow_permission(allowed_roles=[ROLE.ADMIN], level="WORKSPACE")
|
||||
def destroy(self, request, slug, pk):
|
||||
|
||||
@@ -158,3 +158,22 @@ class TestWorkspaceMemberMassAssignment:
|
||||
assert response.status_code == status.HTTP_400_BAD_REQUEST
|
||||
own_member.refresh_from_db()
|
||||
assert own_member.role == 20
|
||||
|
||||
def test_non_integer_role_is_400_not_500(self, attacker_workspace):
|
||||
"""A non-integer role must be a validation error (400), not a 500 — and
|
||||
the guest project-role cascade must not run when the update is invalid."""
|
||||
attacker_ws, attacker = attacker_workspace
|
||||
target = _make_user("bad-role-target@plane.so")
|
||||
target_member = _add_member(attacker_ws, target, role=15)
|
||||
|
||||
client = APIClient()
|
||||
client.force_authenticate(user=attacker)
|
||||
response = client.patch(
|
||||
_member_detail_url(attacker_ws.slug, target_member.id),
|
||||
{"role": "not-a-number"},
|
||||
format="json",
|
||||
)
|
||||
|
||||
assert response.status_code == status.HTTP_400_BAD_REQUEST
|
||||
target_member.refresh_from_db()
|
||||
assert target_member.role == 15
|
||||
|
||||
Reference in New Issue
Block a user