[WIKI-657] refactor: the page permissions in project #7761
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. WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant R as Router / View
participant P as ProjectPagePermission
participant D as DB (Workspace/Project/Page)
C->>R: HTTP request (slug, project_id, [page_id])
activate R
R->>P: has_permission(request, view)
activate P
P->>D: _check_access -> is project member?
alt member
alt page_id present
P->>D: load Page by id in workspace
alt requester is owner
P-->>R: allow
else page.private
P-->>R: _has_private_page_action_access -> deny (default)
else page.public
P-->>R: _has_public_page_action_access -> allow/deny (method+role)
end
else no page_id
P-->>R: allow/deny (method+role)
end
else not member
P-->>R: deny
end
deactivate P
alt allowed
R-->>C: proceed to view (2xx)
else denied
R-->>C: 403 Forbidden
end
deactivate R
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested labels
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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 |
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the page permission system in the project by replacing decorator-based permissions with class-based permissions using a new ProjectPagePermission class.
- Introduces a new
ProjectPagePermissionclass that provides granular access control based on user roles and page visibility - Replaces
@allow_permissiondecorators withpermission_classes = [ProjectPagePermission]across all page-related views - Consolidates permission logic into a centralized permission class with role-based access for different HTTP methods
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/api/plane/app/permissions/page.py | New permission class implementing role-based access control for pages |
| apps/api/plane/app/permissions/init.py | Adds import for the new ProjectPagePermission class |
| apps/api/plane/app/views/page/base.py | Replaces decorator-based permissions with class-based permissions |
| apps/api/plane/app/views/page/version.py | Updates PageVersionEndpoint to use new permission class |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
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/page/base.py (1)
421-457: Extend the new permission to page logs and subpages endpoints.PageLogEndpoint and SubPagesEndpoint operate on page-scoped data but currently have no explicit permission_classes. Apply ProjectPagePermission for consistent enforcement.
class PageLogEndpoint(BaseAPIView): + permission_classes = [ProjectPagePermission] @@ class SubPagesEndpoint(BaseAPIView): + permission_classes = [ProjectPagePermission]I can submit a small follow-up PR to wire these and add minimal tests; want me to draft it?
Also applies to: 459-474
🧹 Nitpick comments (7)
apps/api/plane/app/views/page/base.py (2)
340-341: Nit: clarify the comment grammar.- # if parent archived then page will be un archived breaking hierarchy + # If the parent is archived, un-archiving this page would break the hierarchy; detach the parent.
558-569: Remove redundant inline private-page permission check in PageDuplicateEndpoint.postProjectPagePermission (set on this view) already denies non-owners from accessing private pages; drop the inline block to prevent duplicated logic:
apps/api/plane/app/views/page/base.py:558-564 - # check for permission - if page.access == Page.PRIVATE_ACCESS and page.owned_by_id != request.user.id: - return Response( - {"error": "Permission denied"}, status=status.HTTP_403_FORBIDDEN - )apps/api/plane/app/permissions/page.py (5)
93-101: Comment contradicts logic for POST.The comment says “Only admins can create,” but code allows ADMIN and MEMBER. Align the comment (or tighten the rule).
Option A (update comment to match code):
- # Only admins can create (POST) pages + # Admins and members can create (POST) pagesOption B (enforce admin-only create):
- return ProjectMember.objects.filter( - member_id=user_id, - workspace__slug=slug, - project_id=project_id, - role__in=[ADMIN, MEMBER], - is_active=True, - ).exists() + return ProjectMember.objects.filter( + member_id=user_id, + workspace__slug=slug, + project_id=project_id, + role=ADMIN, + is_active=True, + ).exists()What is the intended policy for page creation at project level?
77-83: Private page handling: confirm intended access beyond owner.Baseline returns False for all non-owners, including admins, even for safe reads. If admins should read private pages or perform certain actions, extend this helper accordingly.
I can provide a variant that permits admin READ on private pages while keeping write restricted to owner/admin as per product policy.
56-66: Minor: consolidate membership lookups to avoid repeated queries.Each branch re-queries
ProjectMember. Consider fetching the role once and reusing it in helpers.Example sketch:
def _get_role(self, user_id, slug, project_id): return ProjectMember.objects.filter( member_id=user_id, workspace__slug=slug, project_id=project_id, is_active=True ).values_list("role", flat=True).first()Then drive decisions off the single
role.
67-75:_check_accessdocstring mentionsNonebut never returns it.Either return
Nonefor “continue” or simplify the doc to True/False.
20-36: Add input validation for missingslug/project_id.If either kwarg is absent, current code proceeds to queries with
Nonevalues. Early-deny on missing identifiers.Example:
slug = view.kwargs.get("slug") project_id = view.kwargs.get("project_id") - page_id = view.kwargs.get("page_id") or view.kwargs.get("pk") + page_id = view.kwargs.get("page_id") or view.kwargs.get("pk") + if not slug or not project_id: + return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/api/plane/app/permissions/__init__.py(1 hunks)apps/api/plane/app/permissions/page.py(1 hunks)apps/api/plane/app/views/page/base.py(8 hunks)apps/api/plane/app/views/page/version.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/api/plane/app/permissions/__init__.py (1)
apps/api/plane/app/permissions/page.py (1)
ProjectPagePermission(14-134)
apps/api/plane/app/views/page/version.py (1)
apps/api/plane/app/permissions/page.py (1)
ProjectPagePermission(14-134)
apps/api/plane/app/permissions/page.py (3)
apps/api/plane/db/models/project.py (1)
ProjectMember(206-250)apps/api/plane/db/models/page.py (1)
Page(19-82)apps/api/plane/app/views/page/base.py (2)
get(461-473)access(262-280)
apps/api/plane/app/views/page/base.py (2)
apps/api/plane/app/views/base.py (1)
BaseViewSet(44-152)apps/api/plane/app/permissions/page.py (1)
ProjectPagePermission(14-134)
⏰ 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). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
apps/api/plane/app/permissions/__init__.py (1)
16-16: Re-export looks good.Importing and exposing ProjectPagePermission via the package surface is correct and matches usages in views.
apps/api/plane/app/views/page/base.py (2)
40-47: Import placement is fine.Local import of ProjectPagePermission keeps the section organized and avoids deep relative paths.
477-478: Applying shared permission to PagesDescriptionViewSet is good.Centralizes access control for description retrieval/updates; aligns with PageViewSet.
apps/api/plane/app/permissions/page.py (1)
42-45: Restrict owner bypass to non-destructive methods
The owner check at the top ofhas_permissionreturnsTruefor every HTTP method, so a non-admin owner can DELETE pages despite the later admin-only delete rule. Gate DELETE (and any other destructive actions) under the owner branch or move the owner check below the method-specific permission logic.
Clarify: should owners be allowed to DELETE pages if they’re not admins, or only GET/PUT/PATCH?
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/page/base.py (1)
494-495: Fix undefined variablepkin page transactionIn the
partial_updatemethod ofPagesDescriptionViewSet, there's a reference to an undefined variablepkwhich should bepage_id.page_transaction.delay( - new_value=request.data, old_value=existing_instance, page_id=pk + new_value=request.data, old_value=existing_instance, page_id=page_id )
♻️ Duplicate comments (4)
apps/api/plane/app/permissions/page.py (3)
52-53: Properly enforce permissions for collection-level operationsWhen no
page_idis provided (list/create operations), the method should still enforce proper role-based access control instead of just deferring to_has_public_page_action_access.The current implementation correctly routes to
_has_public_page_action_accesswhich already handles method-based permissions for list (GET) and create (POST) operations.
28-31: Critical: Handle bothpage_idandpkkwargs to avoid bypassing permission checksThe permission logic only reads
page_idfrom kwargs, but some viewsets still usepkin their routing. Whenpkis used,page_idwill be None and the code will incorrectly treat it as a collection-level operation, potentially bypassing object-level permission checks.Apply this fix to handle both kwargs:
- page_id = view.kwargs.get("page_id") + page_id = view.kwargs.get("page_id") or view.kwargs.get("pk")
39-40: Critical: Add exception handling and project scoping for page lookupThe page lookup has two critical issues:
- It will raise
Page.DoesNotExistcausing a 500 error instead of returning False for proper permission denial- It doesn't filter by project, allowing cross-project access within the same workspace
Apply this fix:
if page_id: - page = Page.objects.get(id=page_id, workspace__slug=slug) + try: + page = Page.objects.get( + id=page_id, + workspace__slug=slug, + projects__id=project_id + ) + except Page.DoesNotExist: + return Falseapps/api/plane/app/views/page/base.py (1)
191-194: Fix misleading error message for Page.DoesNotExistWhen a page doesn't exist, the error message incorrectly states "Access cannot be updated since this page is owned by someone else" which is misleading. Return a proper 404 with a clear message.
except Page.DoesNotExist: return Response( - {"error": "Access cannot be updated since this page is owned by someone else"}, - status=status.HTTP_400_BAD_REQUEST, + {"error": "Page not found"}, + status=status.HTTP_404_NOT_FOUND, )
🧹 Nitpick comments (2)
apps/api/plane/app/permissions/page.py (1)
92-93: Fix comment: Both admins and members can create pagesThe comment says "Only admins can create (POST) pages" but the code allows both ADMIN and MEMBER roles.
- # Only admins can create (POST) pages + # Admins and members can create (POST) pagesapps/api/plane/app/views/page/base.py (1)
403-404: Consider removing redundant decorators on PageFavoriteViewSetThe
@allow_permissiondecorators on PageFavoriteViewSet methods may be redundant since this viewset inherits from BaseViewSet which already haspermission_classes = [IsAuthenticated]. Consider if these decorators are still needed or if the class should useProjectPagePermissionfor consistency.Consider applying class-level permissions like other page-related views:
class PageFavoriteViewSet(BaseViewSet): model = UserFavorite + permission_classes = [ProjectPagePermission] - @allow_permission([ROLE.ADMIN, ROLE.MEMBER]) def create(self, request, slug, project_id, page_id): - @allow_permission([ROLE.ADMIN, ROLE.MEMBER]) def destroy(self, request, slug, project_id, page_id):Also applies to: 414-415
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/api/plane/app/permissions/page.py(1 hunks)apps/api/plane/app/urls/page.py(1 hunks)apps/api/plane/app/views/__init__.py(0 hunks)apps/api/plane/app/views/page/base.py(13 hunks)
💤 Files with no reviewable changes (1)
- apps/api/plane/app/views/init.py
🧰 Additional context used
🧠 Learnings (1)
📚 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/permissions/page.py
🧬 Code graph analysis (3)
apps/api/plane/app/permissions/page.py (3)
apps/api/plane/db/models/project.py (1)
ProjectMember(206-250)apps/api/plane/db/models/page.py (1)
Page(19-82)apps/api/plane/app/views/page/base.py (1)
access(261-279)
apps/api/plane/app/views/page/base.py (4)
apps/api/plane/app/views/base.py (1)
BaseViewSet(44-152)apps/api/plane/app/permissions/page.py (1)
ProjectPagePermission(14-133)apps/api/plane/db/models/page.py (1)
Page(19-82)apps/api/plane/app/serializers/page.py (4)
PageDetailSerializer(126-130)update(105-123)update(239-251)create(58-103)
apps/api/plane/app/urls/page.py (1)
apps/api/plane/app/views/page/base.py (2)
PageViewSet(65-397)PageFavoriteViewSet(400-423)
🔇 Additional comments (4)
apps/api/plane/app/urls/page.py (1)
19-24: LGTM! URL parameter naming is now consistentThe refactoring from
pktopage_idacross all page-related endpoints improves consistency and aligns well with the new permission system's expectations.Also applies to: 27-30, 33-36, 39-42, 45-48, 50-53
apps/api/plane/app/views/page/base.py (3)
68-68: LGTM! Class-level permission properly appliedThe addition of
ProjectPagePermissionat the class level provides consistent permission enforcement across all PageViewSet methods.
427-428: LGTM! Permission classes properly added to PagesDescriptionViewSetThe addition of
ProjectPagePermissionensures consistent permission enforcement for page description operations.
512-512: LGTM! Permission class added to PageDuplicateEndpointThe addition of
ProjectPagePermissionensures proper permission checks for page duplication operations.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/api/plane/app/permissions/page.py (2)
27-31: Prevent permission bypass: also readpkwhen resolvingpage_id.Some routes still use
pk. Current logic treats those as collection routes and skips object-level checks.Apply:
- page_id = view.kwargs.get("page_id") + # Support routes that still use `pk` as the lookup kwarg + page_id = view.kwargs.get("page_id") or view.kwargs.get("pk")Run to locate page endpoints that still use
pk:#!/bin/bash rg -nP -C2 'def\s+\w+\(.*slug,\s*project_id,\s*(pk|page_id)' apps/api/plane/app/views rg -nP 'lookup_url_kwarg\s*=\s*["\']page_id["\']' apps/api/plane/app/views
39-41: Avoid 500s and cross-project leaks on page lookup.Scope by project and handle
DoesNotExist.Apply:
- if page_id: - page = Page.objects.get(id=page_id, workspace__slug=slug) + if page_id: + try: + page = Page.objects.get( + id=page_id, + workspace__slug=slug, + projects__id=project_id, + ) + except Page.DoesNotExist: + return False
🧹 Nitpick comments (2)
apps/api/plane/app/permissions/page.py (2)
82-125: DRY the membership lookups to reduce repeated filters.Build a base queryset once and filter by role per method.
def _check_project_action_access(self, request, slug, project_id): user_id = request.user.id method = request.method + base_qs = ProjectMember.objects.filter( + member_id=user_id, + workspace__slug=slug, + project_id=project_id, + is_active=True, + ) # Only admins can create (POST) pages if method == "POST": - return ProjectMember.objects.filter( - member_id=user_id, - workspace__slug=slug, - project_id=project_id, - role__in=[ADMIN, MEMBER], - is_active=True, - ).exists() + return base_qs.filter(role__in=[ADMIN, MEMBER]).exists() # Safe methods (GET, HEAD, OPTIONS) allowed for all active roles if method in SAFE_METHODS: - return ProjectMember.objects.filter( - member_id=user_id, - workspace__slug=slug, - role__in=[ADMIN, MEMBER, GUEST], - project_id=project_id, - is_active=True, - ).exists() + return base_qs.filter(role__in=[ADMIN, MEMBER, GUEST]).exists() # PUT/PATCH: Admins and members can update if method in ["PUT", "PATCH"]: - return ProjectMember.objects.filter( - member_id=user_id, - workspace__slug=slug, - role__in=[ADMIN, MEMBER], - project_id=project_id, - is_active=True, - ).exists() + return base_qs.filter(role__in=[ADMIN, MEMBER]).exists() # DELETE: Only admins can delete if method == "DELETE": - return ProjectMember.objects.filter( - member_id=user_id, - workspace__slug=slug, - role=ADMIN, - project_id=project_id, - is_active=True, - ).exists() + return base_qs.filter(role=ADMIN).exists()
14-18: Tighten the class docstring to match current behavior.No feature-flag logic exists here; point to the override hook instead.
- Custom permission to control access to pages within a workspace - based on user roles, page visibility (public/private), and feature flags. + Custom permission controlling access to pages within a workspace + based on user roles and page visibility (public/private). + Override `_has_private_page_action_access` for feature-flag specific rules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/app/permissions/page.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/permissions/page.py
🧬 Code graph analysis (1)
apps/api/plane/app/permissions/page.py (3)
apps/api/plane/db/models/project.py (1)
ProjectMember(206-250)apps/api/plane/db/models/page.py (1)
Page(19-82)apps/api/plane/app/views/page/base.py (1)
access(261-279)
🔇 Additional comments (2)
apps/api/plane/app/permissions/page.py (2)
86-95: POST gating: comment vs code mismatch (admins-only vs admins+members).Decide the intended policy and align code or comment.
Option A (admins-only create):
- # Only admins can create (POST) pages + # Only admins can create (POST) pages if method == "POST": - return ProjectMember.objects.filter( + return ProjectMember.objects.filter( member_id=user_id, workspace__slug=slug, - project_id=project_id, - role__in=[ADMIN, MEMBER], + project_id=project_id, + role=ADMIN, is_active=True, ).exists()Option B (admins+members can create) — keep code, fix comment:
- # Only admins can create (POST) pages + # Admins and members can create (POST) pages
50-51: Good: collection routes now use method-based gating.Delegating to
_has_public_page_action_accessavoids blanket allows on list/create.
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 (6)
apps/api/plane/app/views/page/base.py (6)
45-59: Fix raw SQL: wrong table name and datetime type mismatch.
- Django table is likely not "pages". Use Page._meta.db_table.
- Page.archived_at is a DateField; pass a date, not naive datetime.
Apply:
@@ -from datetime import datetime +from datetime import datetime +from django.utils import timezone @@ def unarchive_archive_page_and_descendants(page_id, archived_at): - # Your SQL query - sql = """ - WITH RECURSIVE descendants AS ( - SELECT id FROM pages WHERE id = %s - UNION ALL - SELECT pages.id FROM pages, descendants WHERE pages.parent_id = descendants.id - ) - UPDATE pages SET archived_at = %s WHERE id IN (SELECT id FROM descendants); - """ + table = Page._meta.db_table + sql = f""" + WITH RECURSIVE descendants AS ( + SELECT id FROM {table} WHERE id = %s + UNION ALL + SELECT t.id FROM {table} t, descendants WHERE t.parent_id = descendants.id + ) + UPDATE {table} SET archived_at = %s WHERE id IN (SELECT id FROM descendants); + """ @@ with connection.cursor() as cursor: cursor.execute(sql, [page_id, archived_at])And when calling it (see Lines 318-320), pass a date (timezone.localdate()) and return ISO date (see below).
89-92: Query bugs: conflicting ordering and bad ArrayAgg filter.
- Two order_by calls: the second overrides the first.
- filter=~Q(projects__id=True) is invalid; should check isnull.
@@ - .order_by(self.request.GET.get("order_by", "-created_at")) .prefetch_related("labels") - .order_by("-is_favorite", "-created_at") + .order_by("-is_favorite", "-created_at") @@ - project_ids=Coalesce( - ArrayAgg( - "projects__id", distinct=True, filter=~Q(projects__id=True) - ), - Value([], output_field=ArrayField(UUIDField())), - ), + project_ids=Coalesce( + ArrayAgg( + "projects__id", + distinct=True, + filter=~Q(projects__id__isnull=True), + ), + Value([], output_field=ArrayField(UUIDField())), + ),Also apply the same fix in PageDuplicateEndpoint annotation:
@@ - ArrayAgg( - "projects__id", distinct=True, filter=~Q(projects__id=True) - ), + ArrayAgg( + "projects__id", + distinct=True, + filter=~Q(projects__id__isnull=True), + ),Also applies to: 100-114, 555-566
192-216: Retrieve uses restricted queryset and dereferences None.
- Using get_queryset() excludes non-root pages; child pages 404 incorrectly.
- The guest-gating block uses page before None check → AttributeError risk.
- Prefer ROLE.GUEST constant; use 403 for authorization failures.
- def retrieve(self, request, slug, project_id, page_id=None): - page = self.get_queryset().filter(pk=page_id).first() - project = Project.objects.get(pk=project_id) + def retrieve(self, request, slug, project_id, page_id=None): + page = ( + Page.objects.filter( + pk=page_id, workspace__slug=slug, projects__id=project_id + ).first() + ) + if page is None: + return Response({"error": "Page not found"}, status=status.HTTP_404_NOT_FOUND) + project = Project.objects.get(pk=project_id) @@ - if ( + if ( ProjectMember.objects.filter( workspace__slug=slug, project_id=project_id, member=request.user, - role=5, + role=ROLE.GUEST, is_active=True, ).exists() and not project.guest_view_all_features - and not page.owned_by == request.user + and page.owned_by_id != request.user.id ): - return Response( - {"error": "You are not allowed to view this page"}, - status=status.HTTP_400_BAD_REQUEST, - ) + return Response({"error": "You are not allowed to view this page"}, + status=status.HTTP_403_FORBIDDEN) @@ - if page is None: - return Response( - {"error": "Page not found"}, status=status.HTTP_404_NOT_FOUND - ) - else: + else:Also applies to: 218-221
396-404: Align favorites endpoints with ProjectPagePermission; drop decorators.Use the new permission class for page-level checks and consistency.
class PageFavoriteViewSet(BaseViewSet): model = UserFavorite + permission_classes = [ProjectPagePermission] - @allow_permission([ROLE.ADMIN, ROLE.MEMBER]) def create(self, request, slug, project_id, page_id): @@ - @allow_permission([ROLE.ADMIN, ROLE.MEMBER]) def destroy(self, request, slug, project_id, page_id):Also applies to: 409-411
507-513: Duplicate: handle missing page before access checks..page could be None → 500.
def post(self, request, slug, project_id, page_id): - page = Page.objects.filter( + page = Page.objects.filter( pk=page_id, workspace__slug=slug, projects__id=project_id ).first() + if page is None: + return Response({"error": "Page not found"}, status=status.HTTP_404_NOT_FOUND)
533-541: Bug: variable shadowing of project_id changes payload for copy task.The for-loop reassigns project_id; later used in copy_s3_objects call → wrong project id.
- for project_id in project_ids: + for pid in project_ids: ProjectPage.objects.create( workspace_id=page.workspace_id, - project_id=project_id, + project_id=pid, page_id=page.id, created_by_id=page.created_by_id, updated_by_id=page.updated_by_id, ) @@ copy_s3_objects_of_description_and_assets.delay( entity_name="PAGE", entity_identifier=page.id, - project_id=project_id, + project_id=project_id, # original path param slug=slug, user_id=request.user.id, )Also applies to: 547-553
♻️ Duplicate comments (2)
apps/api/plane/app/views/page/base.py (2)
186-190: Return 404 for missing page in partial_update.The current 400 with ownership message is misleading.
- except Page.DoesNotExist: - return Response( - {"error": "Access cannot be updated since this page is owned by someone else"}, - status=status.HTTP_400_BAD_REQUEST, - ) + except Page.DoesNotExist: + return Response({"error": "Page not found"}, status=status.HTTP_404_NOT_FOUND)
294-298: Wrap Page.objects.get(...) with 404 handling (archive/unarchive/destroy).Uncaught Page.DoesNotExist → 500s.
@@ - page = Page.objects.get( - pk=page_id, workspace__slug=slug, projects__id=project_id - ) + try: + page = Page.objects.get( + pk=page_id, workspace__slug=slug, projects__id=project_id + ) + except Page.DoesNotExist: + return Response({"error": "Page not found"}, status=status.HTTP_404_NOT_FOUND) @@ - page = Page.objects.get( - pk=page_id, workspace__slug=slug, projects__id=project_id - ) + try: + page = Page.objects.get( + pk=page_id, workspace__slug=slug, projects__id=project_id + ) + except Page.DoesNotExist: + return Response({"error": "Page not found"}, status=status.HTTP_404_NOT_FOUND) @@ - page = Page.objects.get( - pk=page_id, workspace__slug=slug, projects__id=project_id - ) + try: + page = Page.objects.get( + pk=page_id, workspace__slug=slug, projects__id=project_id + ) + except Page.DoesNotExist: + return Response({"error": "Page not found"}, status=status.HTTP_404_NOT_FOUND)Also applies to: 322-326, 348-352
🧹 Nitpick comments (4)
apps/api/plane/app/views/page/base.py (4)
318-320: Use timezone-aware date for archive and return ISO date.Avoid naive datetime and match DateField type.
- unarchive_archive_page_and_descendants(page_id, datetime.now()) - - return Response({"archived_at": str(datetime.now())}, status=status.HTTP_200_OK) + archive_date = timezone.localdate() + unarchive_archive_page_and_descendants(page_id, archive_date) + return Response({"archived_at": archive_date.isoformat()}, status=status.HTTP_200_OK)
339-347: Unarchive: minor – consider returning body for symmetry.Optional: return {"archived_at": None} for clients relying on value.
380-385: Consistency: use project_id kw in filters.Prefer project_id=project_id for FK filters to match usage elsewhere.
- UserFavorite.objects.filter( - project=project_id, + UserFavorite.objects.filter( + project_id=project_id,
433-435: Prefer DRF constants for status codes.Use status.HTTP_404_NOT_FOUND for readability.
- return Response({"error": "Page not found"}, status=404) + return Response({"error": "Page not found"}, status=status.HTTP_404_NOT_FOUND)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/app/views/page/base.py(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/app/views/page/base.py (6)
apps/api/plane/app/views/base.py (1)
BaseViewSet(44-152)apps/api/plane/bgtasks/page_version_task.py (1)
page_version(13-49)apps/api/plane/bgtasks/copy_s3_object.py (1)
copy_s3_objects_of_description_and_assets(129-165)apps/api/plane/app/permissions/page.py (1)
ProjectPagePermission(14-137)apps/api/plane/db/models/page.py (1)
Page(19-82)apps/api/plane/app/serializers/page.py (4)
PageDetailSerializer(126-130)update(105-123)update(213-225)create(58-103)
⏰ 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: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
apps/api/plane/app/views/page/base.py (1)
64-64: Good move: centralize page access via ProjectPagePermission.Class-level permission aligns endpoints with the new policy model.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/api/plane/app/permissions/page.py (2)
28-31: Don't bypass object checks: also readpkfrom kwargs.Without a fallback to
pk, routes usingpkskip the object branch and loosen access. Fix:- slug = view.kwargs.get("slug") - page_id = view.kwargs.get("page_id") - project_id = view.kwargs.get("project_id") + slug = view.kwargs.get("slug") + page_id = view.kwargs.get("page_id") or view.kwargs.get("pk") + project_id = view.kwargs.get("project_id")
39-41: Scope by project and handleDoesNotExistto avoid 500s and cross-project leakage.Current
get()may raise and ignores project scope. Patch:- if page_id: - page = Page.objects.get(id=page_id, workspace__slug=slug) + if page_id: + try: + page = ( + Page.objects.only("id", "owned_by_id", "access") + .get(id=page_id, workspace__slug=slug, projects__id=project_id) + ) + except Page.DoesNotExist: + return False
🧹 Nitpick comments (4)
apps/api/plane/app/permissions/page.py (4)
80-86: Clarify private-page hook docstring.Doc says “only owner can access private pages,” but this method returns False (owner is already allowed earlier). Tighten wording:
- """ - Check access to private pages. Override for feature flag logic. - """ - # Base implementation: only owner can access private pages + """ + Check access to private pages for non-owners. Override for feature-flag logic. + Base implementation denies; owner is allowed earlier in has_permission(). + """
90-95: Fix comment to match behavior for create.Code allows ADMIN and MEMBER to POST; comment says “Only admins.” Align:
- # Only admins can create (POST) pages + # Admins and members can create (POST) pages
117-126: Simplify return.No need for the temp variable:
- project_member_exists = self._check_project_action_access(request, role) - if not project_member_exists: - return False - return True + return self._check_project_action_access(request, role)
14-18: Add a denial message for better 403 diagnostics.DRF can surface
messagefrom permissions.class ProjectPagePermission(BasePermission): + message = "You do not have permission to perform this action on this page/project."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/app/permissions/page.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/permissions/page.py
🧬 Code graph analysis (1)
apps/api/plane/app/permissions/page.py (3)
apps/api/plane/db/models/project.py (1)
ProjectMember(206-250)apps/api/plane/db/models/page.py (1)
Page(19-82)apps/api/plane/app/views/page/base.py (1)
access(257-275)
⏰ 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). (1)
- GitHub Check: Build and lint web apps
🔇 Additional comments (3)
apps/api/plane/app/permissions/page.py (3)
42-45: Owner bypass: confirm intended to override method gating.Owner currently bypasses all method rules (e.g., DELETE). If policy requires role/method checks even for owners, move this check below and gate via
_check_project_action_access. I can provide a diff once confirmed.
52-53: LGTM: collection/list/create go through method-based gating.Public/collection paths are correctly funneled to
_has_public_page_action_access.
55-68: No change required — ProjectMember has an inherited workspace FKProjectMember inherits ProjectBaseModel which defines workspace = models.ForeignKey('db.Workspace') (apps/api/plane/db/models/project.py), so using workspace__slug=slug is correct.
Description
this pull request refactors the page permission in project level.
Type of Change
Summary by CodeRabbit
New Features
Refactor
Chores