fix: correct workspace admin permission validation in project member updates#9119
Open
KanteshMurade wants to merge 2 commits into
Open
fix: correct workspace admin permission validation in project member updates#9119KanteshMurade wants to merge 2 commits into
KanteshMurade wants to merge 2 commits into
Conversation
KanteshMurade
requested review from
dheeru0198 and
pablohashescobar
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughRefactors ProjectMemberViewSet.partial_update role-update validation to use the requester's workspace role, add a guard blocking certain upgrades when the target workspace role is 5, and deny non-admins from assigning roles above their own; adds contract tests for admin-allowed and non-admin-denied scenarios. ChangesRole-Assignment Permission Fix
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as ProjectMemberViewSet.partial_update
participant DB as Database
Client->>API: PATCH /projects/{id}/members/{pk} (role change)
API->>DB: fetch target ProjectMember (workspace role)
API->>DB: fetch requesting user's ProjectMember (workspace role)
API->>API: validate requested role vs target.workspace_role and requester.role/admin status
alt allowed (workspace admin or within requester role)
API->>DB: update ProjectMember.role
API-->>Client: 200 OK
else denied
API-->>Client: 400 with error message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/plane/app/views/project/member.py (2)
209-230:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDuplicate workspace role queries with inconsistent variable names.
The code fetches
target_workspace_roleand the requester's workspace role twice:
- Lines 211-218:
requester_workspace_role/is_workspace_admin- Lines 220-229:
requesting_workspace_role/is_requesting_workspace_adminThis causes redundant database queries and creates confusion about which variables to use downstream. The HEAD conflict block uses
is_workspace_adminwhile the other block usesis_requesting_workspace_admin.After resolving the merge conflict, keep only one set of these queries with consistent naming.
Proposed fix (after resolving merge conflict)
- # 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 ).role # Fetch the requester's workspace role to decide if they may bypass project-role checks requester_workspace_role = WorkspaceMember.objects.get( workspace__slug=slug, member=request.user, is_active=True ).role is_workspace_admin = requester_workspace_role == ROLE.ADMIN.value - - # Fetch the workspace role of the project member - target_workspace_role = WorkspaceMember.objects.get( - 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 -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/app/views/project/member.py` around lines 209 - 230, Remove the duplicated WorkspaceMember queries and normalize variable names: keep a single fetch for the target's workspace role into target_workspace_role (from WorkspaceMember with workspace__slug=slug and member=project_member.member) and a single fetch for the requesting user's workspace role into requester_workspace_role (from WorkspaceMember with workspace__slug=slug and member=request.user), then compute is_workspace_admin = requester_workspace_role == ROLE.ADMIN.value and use those variables throughout (replace any uses of requesting_workspace_role or is_requesting_workspace_admin with requester_workspace_role and is_workspace_admin) to eliminate redundant DB calls and make naming consistent.
246-293:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Unresolved merge conflict markers will cause syntax errors.
The file contains literal merge conflict markers (
<<<<<<< HEAD,=======,>>>>>>> c31299f8dd) that must be resolved before this code can run. Python will raise aSyntaxErrorwhen attempting to parse this file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/app/views/project/member.py` around lines 246 - 293, Resolve the literal merge markers by removing the conflict block and keeping a single coherent role-validation flow: delete the lines from <<<<<<< HEAD through ======= and the >>>>>>> marker, and retain the consolidated checks that first validate workspace limits using target_workspace_role and int(request.data.get("role", project_member.role)) against [15, 20], then validate the requester’s authority by comparing int(request.data.get("role", project_member.role)) to requested_project_member.role and using the consistent boolean name is_requesting_workspace_admin (or rename uses of is_workspace_admin to match). Ensure the code uses the same variable names (project_member vs requested_project_member) consistently, converts request.data role to int once, and returns the appropriate Response/status as in the consolidated version.
🧹 Nitpick comments (1)
apps/api/plane/tests/contract/app/test_project_app.py (1)
239-261: 💤 Low valueTest relies on implicit workspace admin setup from fixtures.
The test assumes
create_useris a workspace admin (role 20) through theworkspacefixture, but this isn't explicit in the test. If the fixture behavior changes, this test could fail unexpectedly.Consider adding an explicit assertion or comment documenting this fixture dependency:
# create_user is a workspace admin (role=20) via the workspace fixtureAlternatively, verify the workspace membership explicitly at the start of the test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/tests/contract/app/test_project_app.py` around lines 239 - 261, The test test_workspace_admin_can_promote_member_above_project_role relies implicitly on the create_user being a workspace admin via the workspace fixture; make that dependency explicit by asserting or ensuring the workspace membership/role at the start of the test (e.g., verify WorkspaceMember for create_user has role 20) or add a one-line comment documenting that create_user is a workspace admin; reference the test name test_workspace_admin_can_promote_member_above_project_role, the create_user fixture, and the WorkspaceMember/ProjectMember checks so maintainers can locate and make the verification or comment change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/plane/tests/contract/app/test_project_app.py`:
- Around line 283-284: The test in
apps/api/plane/tests/contract/app/test_project_app.py asserts
response.data["error"] equals "You cannot update a role that is higher than your
own role", which is one side of an unresolved merge in member.py (the
alternative message is "You cannot assign a role equal to or higher than your
own"); update the test to match the final error string chosen in member.py (or
make the assertion resilient by checking for the canonical substring used in the
resolved message) so the assertion aligns with the actual message thrown by the
role-update validation.
---
Outside diff comments:
In `@apps/api/plane/app/views/project/member.py`:
- Around line 209-230: Remove the duplicated WorkspaceMember queries and
normalize variable names: keep a single fetch for the target's workspace role
into target_workspace_role (from WorkspaceMember with workspace__slug=slug and
member=project_member.member) and a single fetch for the requesting user's
workspace role into requester_workspace_role (from WorkspaceMember with
workspace__slug=slug and member=request.user), then compute is_workspace_admin =
requester_workspace_role == ROLE.ADMIN.value and use those variables throughout
(replace any uses of requesting_workspace_role or is_requesting_workspace_admin
with requester_workspace_role and is_workspace_admin) to eliminate redundant DB
calls and make naming consistent.
- Around line 246-293: Resolve the literal merge markers by removing the
conflict block and keeping a single coherent role-validation flow: delete the
lines from <<<<<<< HEAD through ======= and the >>>>>>> marker, and retain the
consolidated checks that first validate workspace limits using
target_workspace_role and int(request.data.get("role", project_member.role))
against [15, 20], then validate the requester’s authority by comparing
int(request.data.get("role", project_member.role)) to
requested_project_member.role and using the consistent boolean name
is_requesting_workspace_admin (or rename uses of is_workspace_admin to match).
Ensure the code uses the same variable names (project_member vs
requested_project_member) consistently, converts request.data role to int once,
and returns the appropriate Response/status as in the consolidated version.
---
Nitpick comments:
In `@apps/api/plane/tests/contract/app/test_project_app.py`:
- Around line 239-261: The test
test_workspace_admin_can_promote_member_above_project_role relies implicitly on
the create_user being a workspace admin via the workspace fixture; make that
dependency explicit by asserting or ensuring the workspace membership/role at
the start of the test (e.g., verify WorkspaceMember for create_user has role 20)
or add a one-line comment documenting that create_user is a workspace admin;
reference the test name
test_workspace_admin_can_promote_member_above_project_role, the create_user
fixture, and the WorkspaceMember/ProjectMember checks so maintainers can locate
and make the verification or comment change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4476ba48-3f71-4f22-aac5-1839bc709b6e
📒 Files selected for processing (2)
apps/api/plane/app/views/project/member.pyapps/api/plane/tests/contract/app/test_project_app.py
KanteshMurade
force-pushed
the
clean-role-fix
branch
from
91e807d to
9f8572c
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/plane/app/views/project/member.py (1)
247-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't block all non-admin project members from role edits.
Lines 249-253 still short-circuit every requester whose project role is below
Admin, so the comparison-based checks below never run for members/guests. That means a project member cannot update someone to a lower role, and the new contract test inapps/api/plane/tests/contract/app/test_project_app.pywill currently get403 {"error": "You do not have permission to update roles"}instead of the intended “above your own role” denial.Suggested minimal fix
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, - ) - # 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( {"error": "You cannot update the role of a member with a role equal to or higher than your own"}, status=status.HTTP_403_FORBIDDEN,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/app/views/project/member.py` around lines 247 - 253, The current check unconditionally blocks anyone below Admin from changing roles; change it to only block unauthorized promotions or edits of admins: when "role" in request.data, read the intended new role (new_role = request.data["role"]) and the requester's project role (e.g., requester_role), then if not is_workspace_admin enforce: if requester_role < ROLE.ADMIN.value and new_role > requester_role (attempting to promote above your role) OR if requested_project_member.role >= ROLE.ADMIN.value and requester_role < ROLE.ADMIN.value (non-admin trying to change an admin), return the 403; use ROLE enum values for comparisons and keep requested_project_member.role and is_workspace_admin checks in place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/api/plane/app/views/project/member.py`:
- Around line 247-253: The current check unconditionally blocks anyone below
Admin from changing roles; change it to only block unauthorized promotions or
edits of admins: when "role" in request.data, read the intended new role
(new_role = request.data["role"]) and the requester's project role (e.g.,
requester_role), then if not is_workspace_admin enforce: if requester_role <
ROLE.ADMIN.value and new_role > requester_role (attempting to promote above your
role) OR if requested_project_member.role >= ROLE.ADMIN.value and requester_role
< ROLE.ADMIN.value (non-admin trying to change an admin), return the 403; use
ROLE enum values for comparisons and keep requested_project_member.role and
is_workspace_admin checks in place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e16afcc6-902b-46d4-8a40-d6d9b9c086e5
📒 Files selected for processing (2)
apps/api/plane/app/views/project/member.pyapps/api/plane/tests/contract/app/test_project_app.py
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes issue #9109 where workspace admins could not promote project members to Admin if their own project role was lower.
The backend permission validation in
ProjectMemberViewSet.partial_updatewas incorrectly checking the workspace role of the target member being edited instead of the requesting user.This change updates the validation logic to use the requesting user's workspace role for permission checks.
Type of Change
Screenshots and Media (if applicable)
N/A
Test Scenarios
References
Fixes #9109
Summary by CodeRabbit
Bug Fixes
Tests