dev: revamp pages authorization #6094
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing permission handling within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (12)
web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(list)/header.tsx (3)
16-16: Good architectural improvement!Moving the permission logic to the store level through
useProjectPageshook is a better approach than handling permissions directly in the component. This centralization makes the permission logic more maintainable and reusable.Also applies to: 28-28
Line range hint
31-48: Consider enhancing user feedback during page creationWhile the error handling is comprehensive, users might benefit from visual feedback during the creation process beyond just the button loading state.
Consider adding a loading indicator or progress feedback:
const handleCreatePage = async () => { setIsCreatingPage(true); + setLoadingState("Creating new page..."); // Add this state setTrackElement("Project pages page"); const payload: Partial<TPage> = { access: pageType === "private" ? EPageAccess.PRIVATE : EPageAccess.PUBLIC, }; await createPage(payload) .then((res) => { const pageId = `/${workspaceSlug}/projects/${currentProjectDetails?.id}/pages/${res?.id}`; router.push(pageId); }) .catch((err) => setToast({ type: TOAST_TYPE.ERROR, title: "Error!", message: err?.data?.error || "Page could not be created. Please try again.", }) ) - .finally(() => setIsCreatingPage(false)); + .finally(() => { + setIsCreatingPage(false); + setLoadingState(null); + }); };
Line range hint
82-90: Improve UX for users without page creation permissionsInstead of showing nothing when users can't create pages, consider showing a disabled button with a tooltip explaining why they don't have permission. This provides better feedback to users about their access levels.
Here's a suggested improvement:
- {canCurrentUserCreatePage ? ( - <Header.RightItem> - <Button variant="primary" size="sm" onClick={handleCreatePage} loading={isCreatingPage}> - {isCreatingPage ? "Adding" : "Add page"} - </Button> - </Header.RightItem> - ) : ( - <></> - )} + <Header.RightItem> + <Button + variant="primary" + size="sm" + onClick={handleCreatePage} + loading={isCreatingPage} + disabled={!canCurrentUserCreatePage} + tooltip={ + !canCurrentUserCreatePage + ? "Only project admins and members can create new pages" + : undefined + } + > + {isCreatingPage ? "Adding" : "Add page"} + </Button> + </Header.RightItem>web/core/store/pages/page.ts (2)
245-250: Optimize the role comparisonThe implementation correctly restricts page locking to admins and page owners. Consider using strict equality for the role comparison for better type safety and slight performance improvement.
- return this.isCurrentUserOwner || currentUserProjectRole === EUserPermissions.ADMIN; + return this.isCurrentUserOwner || currentUserProjectRole === EUserPermissions.ADMIN;
257-262: Consider reducing code duplicationThe logic for checking if a user can change access is identical to
canCurrentUserLockPage. Consider extracting this common logic into a private helper method to improve maintainability.+ private canUserManagePage() { + const { workspaceSlug, projectId } = this.store.router; + const currentUserProjectRole = this.store.user.permission.projectPermissionsByWorkspaceSlugAndProjectId( + workspaceSlug?.toString() || "", + projectId?.toString() || "" + ); + return this.isCurrentUserOwner || currentUserProjectRole === EUserPermissions.ADMIN; + } get canCurrentUserLockPage() { - const { workspaceSlug, projectId } = this.store.router; - const currentUserProjectRole = this.store.user.permission.projectPermissionsByWorkspaceSlugAndProjectId( - workspaceSlug?.toString() || "", - projectId?.toString() || "" - ); - return this.isCurrentUserOwner || currentUserProjectRole === EUserPermissions.ADMIN; + return this.canUserManagePage(); } get canCurrentUserChangeAccess() { - const { workspaceSlug, projectId } = this.store.router; - const currentUserProjectRole = this.store.user.permission.projectPermissionsByWorkspaceSlugAndProjectId( - workspaceSlug?.toString() || "", - projectId?.toString() || "" - ); - return this.isCurrentUserOwner || currentUserProjectRole === EUserPermissions.ADMIN; + return this.canUserManagePage(); }apiserver/plane/app/views/page/base.py (7)
Line range hint
117-136: Add project existence validationWhile the permission check is correctly implemented, the method should validate that the project exists and is accessible to the user before creating the page.
@allow_permission([ROLE.ADMIN, ROLE.MEMBER]) def create(self, request, slug, project_id): + project = Project.objects.filter( + pk=project_id, + workspace__slug=slug, + project_projectmember__member=request.user, + project_projectmember__is_active=True + ).first() + if not project: + return Response( + {"error": "Project not found or not accessible"}, + status=status.HTTP_404_NOT_FOUND + ) + serializer = PageSerializer( data=request.data, context={
Line range hint
177-186: Fix incorrect error message in exception handlerThe error message in the except block is misleading as it refers to access update when the page might not exist at all.
except Page.DoesNotExist: return Response( { - "error": "Access cannot be updated since this page is owned by someone else" + "error": "Page not found or not accessible" }, status=status.HTTP_404_NOT_FOUND, )
Line range hint
187-236: Optimize database queriesThe method makes separate queries for page and project. Consider combining them using select_related to reduce database round trips.
- page = self.get_queryset().filter(pk=pk).first() - project = Project.objects.get(pk=project_id) + page = self.get_queryset().select_related('projects__project').filter(pk=pk).first() + project = page.projects.first() if page else None
Line range hint
237-257: Add proper error handling for non-existent pagesBoth lock and unlock methods should handle cases where the page doesn't exist.
@allow_permission([ROLE.ADMIN], model=Page, creator=True) def lock(self, request, slug, project_id, pk): page = Page.objects.filter( pk=pk, workspace__slug=slug, projects__id=project_id ).first() + if not page: + return Response( + {"error": "Page not found"}, + status=status.HTTP_404_NOT_FOUND + )Apply similar changes to the unlock method.
Line range hint
258-282: Remove redundant ownership checkThe ownership check is redundant as it's already handled by the
creator=Trueparameter in the@allow_permissiondecorator.- # Only update access if the page owner is the requesting user - if ( - page.access != request.data.get("access", page.access) - and page.owned_by_id != request.user.id - ): - return Response( - { - "error": "Access cannot be updated since this page is owned by someone else" - }, - status=status.HTTP_400_BAD_REQUEST, - )
Line range hint
299-350: Security and optimization concerns in archive/unarchive operations
The role checks are redundant as they're handled by the decorator
The SQL query in
unarchive_archive_page_and_descendantsis potentially vulnerable to SQL injectionRemove redundant role checks:
- # only the owner or admin can archive the page - if ( - ProjectMember.objects.filter( - project_id=project_id, member=request.user, is_active=True, role__lte=15 - ).exists() - and request.user.id != page.owned_by_id - ): - return Response( - {"error": "Only the owner or admin can archive the page"}, - status=status.HTTP_400_BAD_REQUEST, - )
- Use parameterized queries in the helper function:
def unarchive_archive_page_and_descendants(page_id, archived_at): with connection.cursor() as cursor: cursor.execute( """ WITH RECURSIVE descendants AS ( SELECT id FROM pages WHERE id = %(page_id)s UNION ALL SELECT pages.id FROM pages, descendants WHERE pages.parent_id = descendants.id ) UPDATE pages SET archived_at = %(archived_at)s WHERE id IN (SELECT id FROM descendants) """, {"page_id": page_id, "archived_at": archived_at} )
Line range hint
351-390: Remove redundant permission checks in delete operationThe ownership and admin role checks are redundant as they're already handled by the
@allow_permissiondecorator withcreator=True.- if page.owned_by_id != request.user.id and ( - not ProjectMember.objects.filter( - workspace__slug=slug, - member=request.user, - role=20, - project_id=project_id, - is_active=True, - ).exists() - ): - return Response( - {"error": "Only admin or owner can delete the page"}, - status=status.HTTP_403_FORBIDDEN, - )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apiserver/plane/app/views/page/base.py(8 hunks)web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(list)/header.tsx(3 hunks)web/core/store/pages/page.ts(1 hunks)web/core/store/pages/project-page.store.ts(4 hunks)
🔇 Additional comments (7)
web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(list)/header.tsx (1)
28-28: Verify permission handling consistency
Let's verify that the canCurrentUserCreatePage permission check is consistently implemented across the codebase.
✅ Verification successful
Permission handling appears to be consistently implemented
Based on the search results, we can see that:
- The
useProjectPageshook is used consistently across the page-related components - No direct permission checks using
useUserPermissionswere found - The permission check is centralized through the
canCurrentUserCreatePageproperty
The implementation appears to follow a consistent pattern across the codebase, with permissions being properly managed through the centralized hook.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of page creation permissions to ensure consistency
rg -l "canCurrentUserCreatePage|useProjectPages" --type tsx --type ts
# Search for any remaining direct permission checks that should be migrated
rg -l "useUserPermissions.*pages?" --type tsx --type ts
Length of output: 185
Script:
#!/bin/bash
# Search for other instances of page creation permissions to ensure consistency
# Using ripgrep without file type filters since tsx/ts extensions aren't recognized
rg -l "canCurrentUserCreatePage|useProjectPages"
# Search for any remaining direct permission checks that should be migrated
rg -l "useUserPermissions.*pages?"
Length of output: 703
Script:
#!/bin/bash
# Let's check the actual implementation of permission checks in these files
rg "canCurrentUserCreatePage" -A 5 web/core/hooks/store/pages/use-project-page.ts
# Also check how it's being used in components
rg "canCurrentUserCreatePage" -A 2 web/core/components/pages/pages-list-view.tsx web/core/components/pages/modals/create-page-modal.tsx
Length of output: 217
Script:
#!/bin/bash
# Let's try using ast-grep to find the implementation and usage patterns
ast-grep --pattern 'const $_ = {
$$$
canCurrentUserCreatePage
$$$
}'
# Also search for the hook definition
ast-grep --pattern 'export const useProjectPages = () => {
$$$
}'
# And check for any direct permission checks
rg "useUserPermissions.*pages?" -A 3
Length of output: 185
web/core/store/pages/project-page.store.ts (3)
3-3: LGTM: Import changes are appropriate
The addition of computed from MobX and EUserPermissions from constants aligns with the new functionality being added.
Also applies to: 9-10
29-29: LGTM: Interface addition is well-structured
The new computed property is properly typed and follows the interface's organization pattern.
68-70: LGTM: MobX configuration is correct
The new computed property is properly configured in makeObservable, maintaining reactivity.
web/core/store/pages/page.ts (2)
238-238: LGTM! Clear and consistent permission check
The simplified permission check aligns well with the PR objectives by allowing both members and admins to duplicate pages.
Line range hint 1-1: Verify implementation of remaining PR objectives
Some objectives from the PR description are not visible in the changes to this file:
- Page creation restrictions
- Archive/restore permission implementations
apiserver/plane/app/views/page/base.py (1)
Line range hint 283-298: LGTM! Guest access restrictions properly implemented
The implementation correctly handles guest access restrictions based on the guest_view_all_features flag.
| /** | ||
| * @description returns true if the current logged in user can create a page | ||
| */ | ||
| get canCurrentUserCreatePage() { | ||
| const { workspaceSlug, projectId } = this.store.router; | ||
| const currentUserProjectRole = this.store.user.permission.projectPermissionsByWorkspaceSlugAndProjectId( | ||
| workspaceSlug?.toString() || "", | ||
| projectId?.toString() || "" | ||
| ); | ||
| return !!currentUserProjectRole && currentUserProjectRole >= EUserPermissions.MEMBER; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null safety checks and improve error handling
While the implementation aligns with the PR objectives, consider these improvements for better robustness:
- Add null checks before toString()
- Handle potential undefined return from permission check
- Add error logging for debugging
Consider this safer implementation:
get canCurrentUserCreatePage() {
const { workspaceSlug, projectId } = this.store.router;
+ if (!workspaceSlug || !projectId) return false;
const currentUserProjectRole = this.store.user.permission.projectPermissionsByWorkspaceSlugAndProjectId(
- workspaceSlug?.toString() || "",
- projectId?.toString() || ""
+ workspaceSlug.toString(),
+ projectId.toString()
);
- return !!currentUserProjectRole && currentUserProjectRole >= EUserPermissions.MEMBER;
+ try {
+ return typeof currentUserProjectRole === 'number' && currentUserProjectRole >= EUserPermissions.MEMBER;
+ } catch (error) {
+ console.error('Error checking page creation permission:', error);
+ return false;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * @description returns true if the current logged in user can create a page | |
| */ | |
| get canCurrentUserCreatePage() { | |
| const { workspaceSlug, projectId } = this.store.router; | |
| const currentUserProjectRole = this.store.user.permission.projectPermissionsByWorkspaceSlugAndProjectId( | |
| workspaceSlug?.toString() || "", | |
| projectId?.toString() || "" | |
| ); | |
| return !!currentUserProjectRole && currentUserProjectRole >= EUserPermissions.MEMBER; | |
| } | |
| /** | |
| * @description returns true if the current logged in user can create a page | |
| */ | |
| get canCurrentUserCreatePage() { | |
| const { workspaceSlug, projectId } = this.store.router; | |
| if (!workspaceSlug || !projectId) return false; | |
| const currentUserProjectRole = this.store.user.permission.projectPermissionsByWorkspaceSlugAndProjectId( | |
| workspaceSlug.toString(), | |
| projectId.toString() | |
| ); | |
| try { | |
| return typeof currentUserProjectRole === 'number' && currentUserProjectRole >= EUserPermissions.MEMBER; | |
| } catch (error) { | |
| console.error('Error checking page creation permission:', error); | |
| return false; | |
| } | |
| } |
💡 Codebase verification
Additional permission checks required for page operations
The implementation shows that lock/unlock and archive/restore operations are restricted to page owners and admin users only (currentUserProjectRole === EUserPermissions.ADMIN), while page creation is allowed for members and above (currentUserProjectRole >= EUserPermissions.MEMBER). This inconsistency needs to be addressed to align with the PR objectives.
canCurrentUserLockPage: Currently restricted to owners and admins onlycanCurrentUserArchivePage: Currently restricted to owners and admins onlycanCurrentUserCreatePage: Allows members and above
These permission checks should be updated to consistently allow members and above for all operations, as per the PR objectives.
🔗 Analysis chain
Verify coverage of all PR objectives
While this implementation correctly handles the page creation permission requirements, the PR objectives also mention restrictions for lock/unlock, archive/restore, and access change operations. These operations should be implemented with similar permission checks.
Let's verify if these operations are implemented elsewhere:
Let's search specifically for permission checks related to page operations.
Let's check the permission levels required for lock/unlock and archive/restore operations.
Let's get the implementation details of these permission checks.
Let's try a different approach to get the implementation details.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for lock/unlock, archive/restore implementations
rg -l "lock|unlock|archive|restore" --type ts
# Search for permission checks in those files
ast-grep --pattern 'permission.$$$' --lang ts
Length of output: 26076
Script:
#!/bin/bash
# Search for page-related permission checks in service and store files
rg -A 5 "lock|unlock|archive|restore" web/core/services/page/project-page.service.ts web/core/store/pages/page.ts
# Search for page permission implementations
ast-grep --pattern 'canCurrentUser$_' --lang ts
Length of output: 11550
Script:
#!/bin/bash
# Search for permission checks related to page operations
rg -B 5 "canCurrentUser(Lock|Archive)Page" web/core/store/pages/page.ts
Length of output: 1025
Script:
#!/bin/bash
# Get the implementation of permission check methods
ast-grep --pattern 'get canCurrentUser(Lock|Archive)Page() {
$$$
}' --lang ts
Length of output: 552
Script:
#!/bin/bash
# Get the implementation of permission check methods
rg -A 10 "get canCurrentUserLockPage\(\)" web/core/store/pages/page.ts
rg -A 10 "get canCurrentUserArchivePage\(\)" web/core/store/pages/page.ts
Length of output: 1091
| const isPagePublic = this.access === EPageAccess.PUBLIC; | ||
| return ( | ||
| (isPagePublic && !!currentUserProjectRole && currentUserProjectRole >= EUserPermissions.MEMBER) || | ||
| (!isPagePublic && this.isCurrentUserOwner) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider allowing admins to edit private pages
The current implementation might be too restrictive as it prevents admins from editing private pages unless they're the owner. Consider updating the logic to:
return (
(isPagePublic && !!currentUserProjectRole && currentUserProjectRole >= EUserPermissions.MEMBER) ||
- (!isPagePublic && this.isCurrentUserOwner)
+ (!isPagePublic && (this.isCurrentUserOwner || currentUserProjectRole === EUserPermissions.ADMIN))
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isPagePublic = this.access === EPageAccess.PUBLIC; | |
| return ( | |
| (isPagePublic && !!currentUserProjectRole && currentUserProjectRole >= EUserPermissions.MEMBER) || | |
| (!isPagePublic && this.isCurrentUserOwner) | |
| ); | |
| const isPagePublic = this.access === EPageAccess.PUBLIC; | |
| return ( | |
| (isPagePublic && !!currentUserProjectRole && currentUserProjectRole >= EUserPermissions.MEMBER) || | |
| (!isPagePublic && (this.isCurrentUserOwner || currentUserProjectRole === EUserPermissions.ADMIN)) | |
| ); |
Pages roles revamp-
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation