Skip to content

fix: correct workspace admin permission validation in project member updates #9119

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
KanteshMurade wants to merge 3 commits into
base: preview
Choose a base branch
from
+95 −0
Open

fix: correct workspace admin permission validation in project member updates #9119

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f8572c
fix: correct workspace admin permission validation in project member …
KanteshMurade May 23, 2026
08043ff
fix: refine role update validation
KanteshMurade May 23, 2026
9b8e33a
test: add regression coverage for project member role updates
KanteshMurade May 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 32 additions & 8 deletions apps/api/plane/app/views/project/member.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ def retrieve(self, request, slug, project_id, pk):
def partial_update(self, request, slug, project_id, pk):
project_member = ProjectMember.objects.get(pk=pk, workspace__slug=slug, project_id=project_id, is_active=True)


# Fetch the target's workspace role (used to cap the new project role)
target_workspace_role = WorkspaceMember.objects.get(
workspace__slug=slug, member=project_member.member, is_active=True
Expand All @@ -216,8 +217,20 @@ def partial_update(self, request, slug, project_id, pk):
).role
is_workspace_admin = requester_workspace_role == ROLE.ADMIN.value

# Fetch the workspace role of the project member
target_workspace_role = WorkspaceMember.objects.get(
Copy link
Copy Markdown
Member

@sriramveeraghanta sriramveeraghanta May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate query. Lines 211 (target_workspace_role) and 215 (requester_workspace_role) on the base branch already compute exactly this. Lines 221–229 re-fetch the same two WorkspaceMember rows and assign aliases (requesting_workspace_role, is_requesting_workspace_admin) for values that are bit-for-bit identical to requester_workspace_role / is_workspace_admin.

Net effect: 2 extra DB round-trips per partial_update and two pairs of synonymous variable names that will eventually diverge in someone's edit. Drop the duplicates and use the existing is_workspace_admin everywhere.

workspace__slug=slug, member=project_member.member, is_active=True
).role

# Fetch the workspace role of the requesting user for permission checks
requesting_workspace_role = WorkspaceMember.objects.get(
workspace__slug=slug, member=request.user, is_active=True
).role
is_requesting_workspace_admin = requesting_workspace_role == ROLE.ADMIN.value

Copy link
Copy Markdown
Member

@sriramveeraghanta sriramveeraghanta May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing whitespace on this line (and stray double-blank lines at 209, 246, 287). Whatever the project lints with (ruff/flake8/black) will almost certainly flag these.


# Check if the user is not editing their own role if they are not an admin
if request.user.id == project_member.member_id and not is_workspace_admin:
if request.user.id == project_member.member_id and not is_requesting_workspace_admin:
return Response(
{"error": "You cannot update your own role"},
status=status.HTTP_400_BAD_REQUEST,
Expand All @@ -230,14 +243,8 @@ def partial_update(self, request, slug, project_id, pk):
is_active=True,
)

if "role" in request.data:
# Only Admins can modify roles
if requested_project_member.role < ROLE.ADMIN.value and not is_workspace_admin:
return Response(
{"error": "You do not have permission to update roles"},
status=status.HTTP_403_FORBIDDEN,
)

Copy link
Copy Markdown
Member

@sriramveeraghanta sriramveeraghanta May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Silent permission expansion. The deletion just above removed:

if requested_project_member.role < ROLE.ADMIN.value and not is_workspace_admin:
    return Response({"error": "You do not have permission to update roles"},
                    status=status.HTTP_403_FORBIDDEN)

That guard hard-blocked any non-admin requester from changing any role at all. Without it, a project MEMBER (role=15, not workspace admin) can now update a project GUEST's role — only the >= requested_project_member.role guard remains. Was this intentional? It's not mentioned in the PR description and isn't covered by either new test.

if "role" in request.data:
# Cannot modify a member whose role is equal to or higher than your own
if project_member.role >= requested_project_member.role and not is_workspace_admin:
return Response(
Expand All @@ -261,6 +268,23 @@ def partial_update(self, request, slug, project_id, pk):
status=status.HTTP_400_BAD_REQUEST,
)

if target_workspace_role in [5] and int(request.data.get("role", project_member.role)) in [15, 20]:
Copy link
Copy Markdown
Member

@sriramveeraghanta sriramveeraghanta May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regression — runs outside if "role" in request.data. This guard is no longer scoped to role updates. With request.data.get("role", project_member.role), when role is not in the request body, it falls back to the existing project_member.role.

Concrete failure: project member with workspace_role=GUEST (5) and pre-existing project_role=MEMBER (15) — e.g. legacy data, or anyone whose workspace role was downgraded after project assignment. Any PATCH that touches only non-role fields (preferences, sort order, view props, etc.) now 400s with "You cannot add a user with role higher than the workspace role".

Move this check back inside the if "role" in request.data: block — note the identical check already exists there at line 265, so this block is also redundant when role is present.

Minor: prefer [ROLE.GUEST.value] / [ROLE.MEMBER.value, ROLE.ADMIN.value] over the magic numbers [5] and [15, 20].

return Response(
{"error": "You cannot add a user with role higher than the workspace role"},
status=status.HTTP_400_BAD_REQUEST,
)

if (
Copy link
Copy Markdown
Member

@sriramveeraghanta sriramveeraghanta May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code. This 400 branch is unreachable:

  • For a non-workspace-admin requester, the existing check at line 258 (new_role >= requested_project_member.role and not is_workspace_admin) already returns 403 for anything strictly greater (>= subsumes >). It fires first.
  • For a workspace admin, is_workspace_admin == is_requesting_workspace_admin == True (same expression, computed twice), so not is_requesting_workspace_admin is False and this branch is skipped.

No input reaches the return Response(..., 400). Either delete it, or — if the intent was to surface a different error message — replace the earlier 403 instead of stacking another check after it.

"role" in request.data
and int(request.data.get("role", project_member.role)) > requested_project_member.role
and not is_requesting_workspace_admin
):
return Response(
{"error": "You cannot update a role that is higher than your own role"},
status=status.HTTP_400_BAD_REQUEST,
)


serializer = ProjectMemberSerializer(project_member, data=request.data, partial=True)

if serializer.is_valid():
Expand Down
58 changes: 58 additions & 0 deletions apps/api/plane/tests/contract/app/test_project_app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,64 @@ def test_create_project_with_all_optional_fields(self, session_client, workspace
assert response_data["network"] == project_data["network"]


@pytest.mark.contract
class TestProjectMemberAPI:
"""Test project member role operations"""

def get_project_member_url(self, workspace_slug: str, project_id: uuid.UUID, pk: uuid.UUID) -> str:
return f"/api/workspaces/{workspace_slug}/projects/{project_id}/members/{pk}/"

@pytest.mark.django_db
def test_workspace_admin_can_promote_member_above_project_role(self, session_client, workspace, create_user):
"""Workspace admins can assign project roles above their own project role."""
project = Project.objects.create(name="Role Project", identifier="RP", workspace=workspace)
requesting_project_member = ProjectMember.objects.create(
project=project, member=create_user, role=5, is_active=True
)

Copy link
Copy Markdown
Member

@sriramveeraghanta sriramveeraghanta May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test passes on preview without the PR. The bug it documents (workspace admin with low project role unable to promote) was already fixed by #9014 (is_workspace_admin = requester_workspace_role == ROLE.ADMIN.value). The test is still valuable as a regression guard, but it shouldn't be framed as proving this PR fixes anything — the production fix is already shipped.

target_user = User.objects.create_user(email="target@example.com", username="target")
WorkspaceMember.objects.create(workspace=workspace, member=target_user, role=15, is_active=True)
target_project_member = ProjectMember.objects.create(
project=project, member=target_user, role=15, is_active=True
)

url = self.get_project_member_url(workspace.slug, project.id, target_project_member.id)
response = session_client.patch(url, {"role": 20}, format="json")

assert response.status_code == status.HTTP_200_OK
target_project_member.refresh_from_db()
assert target_project_member.role == 20

requesting_project_member.refresh_from_db()
assert requesting_project_member.role == 5

@pytest.mark.django_db
def test_project_member_cannot_promote_member_above_own_project_role(self, api_client, workspace):
"""Non-workspace-admin project members cannot assign roles above their own project role."""
project = Project.objects.create(name="Protected Role Project", identifier="PRP", workspace=workspace)

requesting_user = User.objects.create_user(email="requester@example.com", username="requester")
Copy link
Copy Markdown
Member

@sriramveeraghanta sriramveeraghanta May 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion will fail.

With requester project_role=15 patching a target whose project_role=15, the first guard in partial_update fires:

if project_member.role >= requested_project_member.role and not is_workspace_admin:
    return Response({"error": "You cannot update the role of a member with a role equal to or higher than your own"},
                    status=status.HTTP_403_FORBIDDEN)

15 >= 15 is True, not False is True → returns 403 with the equal-to-or-higher message. The new 400 branch this test targets (member.py:277) is unreachable (see comment there).

Both assert response.status_code == 400 and the error string assertion will fail. To make the assertion match the new branch's message, the scenario needs the requester's project_role to be strictly greater than new_role for both prior guards — which contradicts the test name. Either redesign the scenario or assert against the actual 403 + message that the existing guard returns.

WorkspaceMember.objects.create(workspace=workspace, member=requesting_user, role=15, is_active=True)
ProjectMember.objects.create(project=project, member=requesting_user, role=15, is_active=True)

target_user = User.objects.create_user(email="member-target@example.com", username="member-target")
WorkspaceMember.objects.create(workspace=workspace, member=target_user, role=15, is_active=True)
target_project_member = ProjectMember.objects.create(
project=project, member=target_user, role=15, is_active=True
)

api_client.force_authenticate(user=requesting_user)

url = self.get_project_member_url(workspace.slug, project.id, target_project_member.id)
response = api_client.patch(url, {"role": 20}, format="json")

assert response.status_code == status.HTTP_400_BAD_REQUEST
assert response.data["error"] == "You cannot update a role that is higher than your own role"
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

target_project_member.refresh_from_db()
assert target_project_member.role == 15


@pytest.mark.contract
class TestProjectAPIGet(TestProjectBase):
"""Test project GET operations"""
Expand Down
Loading