[WEB-4899] fix: workspace admin cannot delete intake and cycle#7807
[WEB-4899] fix: workspace admin cannot delete intake and cycle#7807
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRemoved an inline owner/project-member check from CycleViewSet.destroy so deletion relies on the admin-only decorator. IntakeIssueViewSet.partial_update now falls back from project-level membership to workspace-level membership ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Router as DRF Router
participant Auth as AdminOnlyPermission
participant View as CycleViewSet.destroy
User->>Router: DELETE /cycles/{id}
Router->>Auth: Check admin-only
alt Authorized
Router->>View: Invoke destroy()
View->>View: Retrieve cycle, cleanup
View-->>User: 204 No Content
else Forbidden
Router-->>User: 403 Forbidden
end
sequenceDiagram
autonumber
actor User
participant View as IntakeIssueViewSet.partial_update
participant PM as ProjectMember
participant WM as WorkspaceMember
participant DB as Database
participant Tasks as BackgroundTasks
User->>View: PATCH /intake/issues/{id}
View->>PM: filter(project=..., member=user).first()
alt Project member found
View->>View: Evaluate project-member role and creator checks
else Not found
View->>WM: filter(workspace=..., member=user).first()
View->>View: Evaluate workspace-admin flag and creator checks
end
alt Allowed
View->>DB: Apply partial update (assemble issue_data from request or existing fields)
View->>Tasks: issue_description_version_task(updated_issue=current_instance, issue_id=str(pk))
View-->>User: 200 OK
else Forbidden
View-->>User: 403 Forbidden
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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 |
|
Linked to Plane Work Item(s)
This comment was auto-generated by Plane |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/intake/base.py (2)
343-351: Bug: passing a model instance tointake_idfilter; also missing 404 when intake not found.
intake_idis a scalar FK column; passing an Intake instance (intake_id=intake_id) can raise a type error. Also guard for a missing intake before the.get().Apply this diff:
- intake_id = Intake.objects.filter( + intake = Intake.objects.filter( workspace__slug=slug, project_id=project_id ).first() - intake_issue = IntakeIssue.objects.get( + if not intake: + return Response({"error": "Intake not found"}, status=status.HTTP_404_NOT_FOUND) + intake_issue = IntakeIssue.objects.get( issue_id=pk, workspace__slug=slug, project_id=project_id, - intake_id=intake_id, + intake_id=intake.id, )
447-451: Fix role-check: include Members (use >= or ROLE.MEMBER.value)ROLE mapping: Admin=20, Member=15, Guest=5 — current
if project_member.role > 15excludes MEMBER (15). Change toproject_member.role >= 15or compare against the enum constant (e.g.ROLE.MEMBER.value) and remove the magic number.Location: apps/api/plane/app/views/intake/base.py:447-451. Also update the identical check in apps/api/plane/api/views/intake.py:408-411.
🧹 Nitpick comments (3)
apps/api/plane/app/views/intake/base.py (3)
366-373: Use 403 Forbidden for authz failures, not 400.This is an authorization failure, not a bad request. Return 403 for consistency with DRF and other endpoints in this file.
- return Response( - {"error": "You cannot edit intake issues"}, - status=status.HTTP_400_BAD_REQUEST, - ) + return Response( + {"error": "You are not allowed to edit intake issues"}, + status=status.HTTP_403_FORBIDDEN, + )
608-612: Guard against None before deleting related Issue.
Issue.objects.filter(...).first()may return None; calling.delete()would raise. Add a null check.- issue = Issue.objects.filter( + issue = Issue.objects.filter( workspace__slug=slug, project_id=project_id, pk=pk ).first() - issue.delete() + if issue: + issue.delete()
71-75: Possible None dereference in IntakeViewSet.list.
intake = self.get_queryset().first()can be None; serializing None can break. Mirror the 404 handling used elsewhere.- intake = self.get_queryset().first() - return Response(IntakeSerializer(intake).data, status=status.HTTP_200_OK) + intake = self.get_queryset().first() + if not intake: + return Response({"error": "Intake not found"}, status=status.HTTP_404_NOT_FOUND) + return Response(IntakeSerializer(intake).data, status=status.HTTP_200_OK)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/plane/app/views/cycle/base.py(0 hunks)apps/api/plane/app/views/intake/base.py(2 hunks)
💤 Files with no reviewable changes (1)
- apps/api/plane/app/views/cycle/base.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: NarayanBavisetti
PR: makeplane/plane#7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
📚 Learning: 2025-07-23T18:18:06.875Z
Learnt from: NarayanBavisetti
PR: makeplane/plane#7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
Applied to files:
apps/api/plane/app/views/intake/base.py
🧬 Code graph analysis (1)
apps/api/plane/app/views/intake/base.py (2)
apps/api/plane/db/models/workspace.py (1)
WorkspaceMember(202-234)apps/api/plane/db/models/project.py (1)
ProjectMember(212-256)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
apps/api/plane/app/views/intake/base.py (1)
31-32: Import of WorkspaceMember looks correct.Needed for the workspace-level fallback. No concerns.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
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/intake/base.py (1)
343-351: Guard against missing Intake/issue and fix misuse of intake_id
.get()onIntakeIssuewill 500 when not found; return 404 instead.- You pass an Intake object into
intake_id=...; useintake_id.id.Apply this diff:
- intake_id = Intake.objects.filter( - workspace__slug=slug, project_id=project_id - ).first() - intake_issue = IntakeIssue.objects.get( - issue_id=pk, - workspace__slug=slug, - project_id=project_id, - intake_id=intake_id, - ) + intake_id = Intake.objects.filter( + workspace__slug=slug, project_id=project_id + ).first() + if not intake_id: + return Response( + {"error": "Intake not found"}, + status=status.HTTP_404_NOT_FOUND, + ) + intake_issue = ( + IntakeIssue.objects.filter( + issue_id=pk, + workspace__slug=slug, + project_id=project_id, + intake_id=intake_id.id, + ).first() + ) + if not intake_issue: + return Response( + {"error": "Intake issue not found"}, + status=status.HTTP_404_NOT_FOUND, + )
♻️ Duplicate comments (2)
apps/api/plane/app/views/intake/base.py (2)
375-381: Remove incorrect role check and wrong status code
- Refers to
project_member.rolewhich may not exist (workspace-admin-only path).- Logic is inverted and returns 400 for an authorization failure; should be 403.
Apply this diff:
- if (project_member.role <= 5 or not is_workspace_admin) and str( - intake_issue.created_by_id - ) != str(request.user.id): - return Response( - {"error": "You cannot edit intake issues"}, - status=status.HTTP_400_BAD_REQUEST, - ) + # Authorization is already enforced above; no extra check needed here.
411-411: Guest-only edit rule is inverted; use is_adminCurrent condition lets admins and everyone with role<=5 into the restricted path. Instead, restrict the limited edit path to non-admins (i.e., creators who aren’t admins).
Apply this diff:
- if project_member.role <= 5 or is_workspace_admin: + if not is_admin:
🧹 Nitpick comments (1)
apps/api/plane/app/views/intake/base.py (1)
341-373: Consistency: rely on decorator for coarse auth; keep fine-grained rules hereGiven
@allow_permission(allowed_roles=[ROLE.ADMIN], creator=True, model=Issue), avoid re-implementing coarse checks inline. The simplifiedis_admin/is_creatorabove is fine if you need it for field-level control, but don’t diverge from decorator semantics.Would you like me to push a follow-up commit that removes the redundant coarse checks and adds tests for:
- workspace admin (no project membership) can PATCH attributes
- creator (non-admin) can only change name/description
- non-creator member is forbidden (403)?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/app/views/intake/base.py(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: NarayanBavisetti
PR: makeplane/plane#7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
📚 Learning: 2025-07-23T18:18:06.875Z
Learnt from: NarayanBavisetti
PR: makeplane/plane#7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
Applied to files:
apps/api/plane/app/views/intake/base.py
🧬 Code graph analysis (1)
apps/api/plane/app/views/intake/base.py (2)
apps/api/plane/db/models/workspace.py (1)
WorkspaceMember(202-234)apps/api/plane/db/models/project.py (1)
ProjectMember(212-256)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
apps/api/plane/app/views/intake/base.py (1)
31-31: Import looks good
WorkspaceMemberis used below; import is correct.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
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/intake/base.py (2)
343-351: Bug: filtering by intake_id passes a model instance, not an ID.This will raise a TypeError; use intake.id or filter by intake.
Apply this diff:
- intake_id = Intake.objects.filter( + intake = Intake.objects.filter( workspace__slug=slug, project_id=project_id ).first() - intake_issue = IntakeIssue.objects.get( + if not intake: + return Response({"error": "Intake not found"}, status=status.HTTP_404_NOT_FOUND) + intake_issue = IntakeIssue.objects.get( issue_id=pk, workspace__slug=slug, project_id=project_id, - intake_id=intake_id, + intake_id=intake.id, )
413-420: Workspace admins are accidentally restricted to name/description only. Invert the condition.Gate the “limited fields” path on not is_admin.
Apply this diff:
- if project_member.role <= 5 or is_workspace_admin: + if not is_admin: issue_data = { "name": issue_data.get("name", issue.name), "description_html": issue_data.get( "description_html", issue.description_html ), "description": issue_data.get("description", issue.description), }
♻️ Duplicate comments (3)
apps/api/plane/app/views/intake/base.py (3)
376-383: Remove redundant and brittle check; it can 400 for valid creators and NPE when no project_member.Decorator already enforces admin-or-creator. This block mixes magic numbers and will AttributeError when project_member is None.
Apply this diff:
- # Only project members admins and created_by users can access this endpoint - if (project_member.role <= 5 or not is_workspace_admin) and str( - intake_issue.created_by_id - ) != str(request.user.id): - return Response( - {"error": "You cannot edit intake issues"}, - status=status.HTTP_400_BAD_REQUEST, - ) + # Authorization handled by decorator; field-level rules below.
353-374: Authorization gate is inverted and duplicates the decorator. Normalize to is_admin/is_creator.Condition
if not project_member or not is_workspace_admindenies valid users; also redundant with @allow_permission. Compute is_admin once; drop the extra 403.Apply this diff:
- project_member = ProjectMember.objects.filter( + project_member = ProjectMember.objects.filter( workspace__slug=slug, project_id=project_id, member=request.user, is_active=True, - ).first() - - is_workspace_admin = False - - if not project_member: - is_workspace_admin = WorkspaceMember.objects.filter( - workspace__slug=slug, - is_active=True, - member=request.user, - role=ROLE.ADMIN.value, - ).exists() - - if not project_member or not is_workspace_admin: - return Response( - {"error": "Only admin or creator can update the intake work items"}, - status=status.HTTP_403_FORBIDDEN, - ) + ).first() + is_project_admin = ProjectMember.objects.filter( + workspace__slug=slug, + project_id=project_id, + member=request.user, + role=ROLE.ADMIN.value, + is_active=True, + ).exists() + is_workspace_admin = WorkspaceMember.objects.filter( + workspace__slug=slug, + member=request.user, + role=ROLE.ADMIN.value, + is_active=True, + ).exists() + is_admin = is_project_admin or is_workspace_admin + is_creator = str(intake_issue.created_by_id) == str(request.user.id)
457-459: Replace magic role thresholds with is_admin.
role > 15is opaque; rely on the boolean computed above.Apply this diff:
- # Only project admins can edit intake issue attributes - if project_member.role > 15 or is_workspace_admin: + # Only admins can edit intake issue attributes + if is_admin:
🧹 Nitpick comments (1)
apps/api/plane/app/views/intake/base.py (1)
343-459: Remove numeric role comparisons; rely on decorator-provided is_admin/is_creator for branchingFound numeric role comparisons in apps/api/plane/app/views/intake/base.py at lines 377 (project_member.role <= 5), 413 (project_member.role <= 5) and 458 (project_member.role > 15). Replace these with the boolean flags populated by @allow_permission (is_admin / is_creator), keep those booleans only for field-level branching, and remove the duplicate 403 checks inside partial_update.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/app/views/intake/base.py(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: NarayanBavisetti
PR: makeplane/plane#7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
📚 Learning: 2025-07-23T18:18:06.875Z
Learnt from: NarayanBavisetti
PR: makeplane/plane#7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
Applied to files:
apps/api/plane/app/views/intake/base.py
🧬 Code graph analysis (1)
apps/api/plane/app/views/intake/base.py (2)
apps/api/plane/db/models/workspace.py (1)
WorkspaceMember(202-234)apps/api/plane/db/models/project.py (1)
ProjectMember(212-256)
🔇 Additional comments (1)
apps/api/plane/app/views/intake/base.py (1)
31-31: Import looks good.Adding WorkspaceMember here is appropriate for workspace‑level checks.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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 (1)
apps/api/plane/app/views/intake/base.py (1)
343-351: Bug: Wrong FK lookup and missing 404 guard for Intake
- Passing a model instance to
intake_id=is invalid; use the UUID (.id) or theintake=FK field.- If no Intake exists,
.get()on IntakeIssue will 500.Fix with:
- intake_id = Intake.objects.filter( + intake_id = Intake.objects.filter( workspace__slug=slug, project_id=project_id ).first() + if not intake_id: + return Response( + {"error": "Intake not found"}, + status=status.HTTP_404_NOT_FOUND, + ) intake_issue = IntakeIssue.objects.get( issue_id=pk, workspace__slug=slug, project_id=project_id, - intake_id=intake_id, + intake_id=intake_id.id, )
♻️ Duplicate comments (1)
apps/api/plane/app/views/intake/base.py (1)
457-459: Fix inverted/magic-number role check; gate attribute edits by is_adminReplace brittle numeric role logic with the derived boolean and ensure workspace admins can edit attributes.
- # Only project admins can edit intake issue attributes - if (project_member and project_member.role > 15) or is_workspace_admin: + # Only admins can edit intake issue attributes + if is_admin:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/app/views/intake/base.py(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: NarayanBavisetti
PR: makeplane/plane#7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
📚 Learning: 2025-07-23T18:18:06.875Z
Learnt from: NarayanBavisetti
PR: makeplane/plane#7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
Applied to files:
apps/api/plane/app/views/intake/base.py
🧬 Code graph analysis (1)
apps/api/plane/app/views/intake/base.py (2)
apps/api/plane/db/models/workspace.py (1)
WorkspaceMember(202-234)apps/api/plane/db/models/project.py (1)
ProjectMember(212-256)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
apps/api/plane/app/views/intake/base.py (2)
31-31: Add WorkspaceMember import — OKImport looks correct and scoped to the new checks.
413-419: Issue data fallback — OKMerging with existing fields is correct and avoids nulling unspecified attributes.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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 (1)
apps/api/plane/app/views/intake/base.py (1)
346-351: Bug: Wrong ORM filter uses model instance for*_idfield
intake_idhere is an Intake instance (from.first()), but you pass it tointake_id=…. Django expects the PK value; passing the model instance can raise a ValueError at runtime.Apply:
- intake_issue = IntakeIssue.objects.get( + intake_issue = IntakeIssue.objects.get( issue_id=pk, workspace__slug=slug, project_id=project_id, - intake_id=intake_id, + intake_id=intake_id.id, )
♻️ Duplicate comments (1)
apps/api/plane/app/views/intake/base.py (1)
457-458: Admin‑only attribute edits: replace magic role threshold with clearis_adminNumeric role comparisons are brittle and currently invert intent. Use the derived
is_admin.- # Only project admins can edit intake issue attributes - if (project_member and project_member.role > 15) or is_workspace_admin: + # Only admins can edit intake issue attributes + if is_admin:Optional: if you want to clearly signal denial, return 403 instead of silently echoing the current state in the
elsebranch.
🧹 Nitpick comments (1)
apps/api/plane/app/views/intake/base.py (1)
31-31: Import only if inline workspace‑admin logic remainsIf we drop the inline permission logic (recommended below), remove this import to avoid dead code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/app/views/intake/base.py(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: NarayanBavisetti
PR: makeplane/plane#7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
📚 Learning: 2025-07-23T18:18:06.875Z
Learnt from: NarayanBavisetti
PR: makeplane/plane#7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
Applied to files:
apps/api/plane/app/views/intake/base.py
🧬 Code graph analysis (1)
apps/api/plane/app/views/intake/base.py (2)
apps/api/plane/db/models/workspace.py (1)
WorkspaceMember(202-234)apps/api/plane/db/models/project.py (1)
ProjectMember(212-256)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
apps/api/plane/app/views/intake/base.py (1)
413-420: LGTM: Safe field fallback for partial updatesMerges incoming issue fields with existing values; concise and correct.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/api/plane/app/views/intake/base.py (4)
376-383: Incorrect auth logic and 400 status; delete this block.
The boolean is wrong and 400 is not for auth. Removing it is covered by relying on the decorator + the refactor above.Apply:
- if ( - (project_member and project_member.role <= 5) and not is_workspace_admin - ) and str(intake_issue.created_by_id) != str(request.user.id): - return Response( - {"error": "You cannot edit intake issues"}, - status=status.HTTP_400_BAD_REQUEST, - )
413-421: Field restriction uses magic numbers; tie to is_admin instead.
This currently keys offproject_member.role <= 5. Use the derived flag to keep intent clear and future‑proof.Apply:
- if project_member and project_member.role <= 5: + if not is_admin: issue_data = { "name": issue_data.get("name", issue.name), "description_html": issue_data.get( "description_html", issue.description_html ), "description": issue_data.get("description", issue.description), }
353-374: Remove redundant gate; derive is_admin/is_creator once (decorator already enforces access).
This block re-gates “admin or creator” and can wrongly block creators who aren’t project members. Replace with a single privilege derivation used later.Apply:
- project_member = ProjectMember.objects.filter( - workspace__slug=slug, - project_id=project_id, - member=request.user, - is_active=True, - ).first() - - is_workspace_admin = False - - if not project_member: - is_workspace_admin = WorkspaceMember.objects.filter( - workspace__slug=slug, - is_active=True, - member=request.user, - role=ROLE.ADMIN.value, - ).exists() - - if not project_member and not is_workspace_admin: - return Response( - {"error": "Only admin or creator can update the intake work items"}, - status=status.HTTP_403_FORBIDDEN, - ) + # Base permission (admin OR creator) is enforced by the decorator above. + # Derive flags only for downstream admin-only behavior. + is_project_admin = ProjectMember.objects.filter( + workspace__slug=slug, + project_id=project_id, + member=request.user, + role=ROLE.ADMIN.value, + is_active=True, + ).exists() + is_workspace_admin = WorkspaceMember.objects.filter( + workspace__slug=slug, + member=request.user, + role=ROLE.ADMIN.value, + is_active=True, + ).exists() + is_admin = is_project_admin or is_workspace_admin + is_creator = str(intake_issue.created_by_id) == str(request.user.id)
458-459: Admin check inverted: non-admins can edit attributes.
The condition allows high-role-number users; flip tois_admin.Apply:
- # Only project admins can edit intake issue attributes - if (project_member and project_member.role > 15) or is_workspace_admin: + # Only admins (project or workspace) can edit intake issue attributes + if is_admin:
🧹 Nitpick comments (1)
apps/api/plane/app/views/intake/base.py (1)
346-351: ForeignKey lookup usesintake_idwith a model instance.
Pass the FK object tointake=or its.idtointake_id=.Apply:
- intake_issue = IntakeIssue.objects.get( - issue_id=pk, - workspace__slug=slug, - project_id=project_id, - intake_id=intake_id, - ) + intake_issue = IntakeIssue.objects.get( + issue_id=pk, + workspace__slug=slug, + project_id=project_id, + intake=intake_id, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/app/views/intake/base.py(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: NarayanBavisetti
PR: makeplane/plane#7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
📚 Learning: 2025-07-23T18:18:06.875Z
Learnt from: NarayanBavisetti
PR: makeplane/plane#7460
File: apps/api/plane/app/serializers/draft.py:112-122
Timestamp: 2025-07-23T18:18:06.875Z
Learning: In the Plane codebase serializers, workspace_id is not consistently passed in serializer context, so parent issue validation in DraftIssueCreateSerializer only checks project_id rather than both workspace_id and project_id. The existing project member authentication system already validates that users can only access projects they belong to, providing sufficient security without risking breaking functionality by adding workspace_id validation where the context might not be available.
Applied to files:
apps/api/plane/app/views/intake/base.py
🧬 Code graph analysis (1)
apps/api/plane/app/views/intake/base.py (2)
apps/api/plane/db/models/workspace.py (1)
WorkspaceMember(202-234)apps/api/plane/db/models/project.py (1)
ProjectMember(212-256)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
apps/api/plane/app/views/intake/base.py (1)
31-31: Import of WorkspaceMember looks good.
Used later for admin checks; no concerns.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes workspace admin permissions for intake and cycle operations by modifying permission checks in the respective viewsets. The changes ensure workspace admins can properly delete cycles and update intake issues, even when they're not direct project members.
- Removed redundant permission checks for cycle deletion (now handled by decorator)
- Added workspace admin fallback permission for intake issue updates
- Improved error handling for non-project members accessing intake functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/api/plane/app/views/intake/base.py | Enhanced intake issue update permissions to include workspace admin checks |
| apps/api/plane/app/views/cycle/base.py | Removed duplicate permission validation for cycle deletion |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if ( | ||
| (project_member and project_member.role <= ROLE.GUEST.value) | ||
| and not is_workspace_admin | ||
| ) and str(intake_issue.created_by_id) != str(request.user.id): |
There was a problem hiding this comment.
[nitpick] The complex conditional logic with nested parentheses is hard to read and maintain. Consider extracting this into a helper method or breaking it into separate conditions with descriptive variable names.
| if ( | |
| (project_member and project_member.role <= ROLE.GUEST.value) | |
| and not is_workspace_admin | |
| ) and str(intake_issue.created_by_id) != str(request.user.id): | |
| if not self.can_edit_intake_issue(project_member, is_workspace_admin, intake_issue, request): |
…lane#7807) * fix: permission check on viewset * chore: check workspace admin * chore: initiative is_workspace_admin before if condition * chore: project member check * fix: if conditions * chore: add condition for guests to only edit description and name * fix: use ROLE enum instead of magic numbers * chore: remove if condition
Description
This PR modifies the permission checks in the intake and cycle viewsets, since these are already handled in the decorator.
Type of Change
Summary by CodeRabbit