Skip to content

dev: revamp pages authorization #6094

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
pushya22 merged 1 commit into from
Dec 2, 2024
Merged

dev: revamp pages authorization #6094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions apiserver/plane/app/views/page/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ def get_queryset(self):
.distinct()
)

@allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST])
@allow_permission([ROLE.ADMIN, ROLE.MEMBER])
def create(self, request, slug, project_id):
serializer = PageSerializer(
data=request.data,
Expand All @@ -134,7 +134,7 @@ def create(self, request, slug, project_id):
return Response(serializer.data, status=status.HTTP_201_CREATED)
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)

@allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST])
@allow_permission([ROLE.ADMIN, ROLE.MEMBER])
def partial_update(self, request, slug, project_id, pk):
try:
page = Page.objects.get(
Expand Down Expand Up @@ -234,7 +234,7 @@ def retrieve(self, request, slug, project_id, pk=None):
)
return Response(data, status=status.HTTP_200_OK)

@allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST])
@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
Expand All @@ -244,7 +244,7 @@ def lock(self, request, slug, project_id, pk):
page.save()
return Response(status=status.HTTP_204_NO_CONTENT)

@allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST])
@allow_permission([ROLE.ADMIN], model=Page, creator=True)
def unlock(self, request, slug, project_id, pk):
page = Page.objects.filter(
pk=pk, workspace__slug=slug, projects__id=project_id
Expand All @@ -255,7 +255,7 @@ def unlock(self, request, slug, project_id, pk):

return Response(status=status.HTTP_204_NO_CONTENT)

@allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST])
@allow_permission([ROLE.ADMIN], model=Page, creator=True)
def access(self, request, slug, project_id, pk):
access = request.data.get("access", 0)
page = Page.objects.filter(
Expand Down Expand Up @@ -296,7 +296,7 @@ def list(self, request, slug, project_id):
pages = PageSerializer(queryset, many=True).data
return Response(pages, status=status.HTTP_200_OK)

@allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST])
@allow_permission([ROLE.ADMIN], model=Page, creator=True)
def archive(self, request, slug, project_id, pk):
page = Page.objects.get(pk=pk, workspace__slug=slug, projects__id=project_id)

Expand All @@ -323,7 +323,7 @@ def archive(self, request, slug, project_id, pk):

return Response({"archived_at": str(datetime.now())}, status=status.HTTP_200_OK)

@allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST])
@allow_permission([ROLE.ADMIN], model=Page, creator=True)
def unarchive(self, request, slug, project_id, pk):
page = Page.objects.get(pk=pk, workspace__slug=slug, projects__id=project_id)

Expand All @@ -348,7 +348,7 @@ def unarchive(self, request, slug, project_id, pk):

return Response(status=status.HTTP_204_NO_CONTENT)

@allow_permission([ROLE.ADMIN], creator=True, model=Page)
@allow_permission([ROLE.ADMIN], model=Page, creator=True)
def destroy(self, request, slug, project_id, pk):
page = Page.objects.get(pk=pk, workspace__slug=slug, projects__id=project_id)

Expand Down
15 changes: 3 additions & 12 deletions web/app/[workspaceSlug]/(projects)/projects/(detail)/[projectId]/pages/(list)/header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ import { BreadcrumbLink, Logo } from "@/components/common";
// constants
import { EPageAccess } from "@/constants/page";
// hooks
import { useEventTracker, useProject, useProjectPages, useUserPermissions } from "@/hooks/store";
// plane web hooks
import { EUserPermissions, EUserPermissionsLevel } from "@/plane-web/constants/user-permissions";
import { useEventTracker, useProject, useProjectPages } from "@/hooks/store";

export const PagesListHeader = observer(() => {
// states
Expand All @@ -26,16 +24,9 @@ export const PagesListHeader = observer(() => {
const searchParams = useSearchParams();
const pageType = searchParams.get("type");
// store hooks
const { allowPermissions } = useUserPermissions();

const { currentProjectDetails, loader } = useProject();
const { createPage } = useProjectPages();
const { canCurrentUserCreatePage, createPage } = useProjectPages();
const { setTrackElement } = useEventTracker();
// auth
const canUserCreatePage = allowPermissions(
[EUserPermissions.ADMIN, EUserPermissions.MEMBER, EUserPermissions.GUEST],
EUserPermissionsLevel.PROJECT
);
// handle page create
const handleCreatePage = async () => {
setIsCreatingPage(true);
Expand Down Expand Up @@ -88,7 +79,7 @@ export const PagesListHeader = observer(() => {
</Breadcrumbs>
</div>
</Header.LeftItem>
{canUserCreatePage ? (
{canCurrentUserCreatePage ? (
<Header.RightItem>
<Button variant="primary" size="sm" onClick={handleCreatePage} loading={isCreatingPage}>
{isCreatingPage ? "Adding" : "Add page"}
Expand Down
27 changes: 18 additions & 9 deletions web/core/store/pages/page.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,47 +215,58 @@ export class Page implements IPage {
*/
get canCurrentUserEditPage() {
const { workspaceSlug, projectId } = this.store.router;

const currentUserProjectRole = this.store.user.permission.projectPermissionsByWorkspaceSlugAndProjectId(
workspaceSlug?.toString() || "",
projectId?.toString() || ""
);
return this.isCurrentUserOwner || (!!currentUserProjectRole && currentUserProjectRole >= EUserPermissions.MEMBER);
const isPagePublic = this.access === EPageAccess.PUBLIC;
return (
(isPagePublic && !!currentUserProjectRole && currentUserProjectRole >= EUserPermissions.MEMBER) ||
(!isPagePublic && this.isCurrentUserOwner)
);
Comment on lines +222 to +226
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 26, 2024

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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))
);

}

/**
* @description returns true if the current logged in user can create a duplicate the page
*/
get canCurrentUserDuplicatePage() {
const { workspaceSlug, projectId } = this.store.router;

const currentUserProjectRole = this.store.user.permission.projectPermissionsByWorkspaceSlugAndProjectId(
workspaceSlug?.toString() || "",
projectId?.toString() || ""
);
return this.isCurrentUserOwner || (!!currentUserProjectRole && currentUserProjectRole >= EUserPermissions.MEMBER);
return !!currentUserProjectRole && currentUserProjectRole >= EUserPermissions.MEMBER;
}

/**
* @description returns true if the current logged in user can lock the page
*/
get canCurrentUserLockPage() {
return this.isCurrentUserOwner;
const { workspaceSlug, projectId } = this.store.router;
const currentUserProjectRole = this.store.user.permission.projectPermissionsByWorkspaceSlugAndProjectId(
workspaceSlug?.toString() || "",
projectId?.toString() || ""
);
return this.isCurrentUserOwner || currentUserProjectRole === EUserPermissions.ADMIN;
}

/**
* @description returns true if the current logged in user can change the access of the page
*/
get canCurrentUserChangeAccess() {
return this.isCurrentUserOwner;
const { workspaceSlug, projectId } = this.store.router;
const currentUserProjectRole = this.store.user.permission.projectPermissionsByWorkspaceSlugAndProjectId(
workspaceSlug?.toString() || "",
projectId?.toString() || ""
);
return this.isCurrentUserOwner || currentUserProjectRole === EUserPermissions.ADMIN;
}

/**
* @description returns true if the current logged in user can archive the page
*/
get canCurrentUserArchivePage() {
const { workspaceSlug, projectId } = this.store.router;

const currentUserProjectRole = this.store.user.permission.projectPermissionsByWorkspaceSlugAndProjectId(
workspaceSlug?.toString() || "",
projectId?.toString() || ""
Expand All @@ -268,7 +279,6 @@ export class Page implements IPage {
*/
get canCurrentUserDeletePage() {
const { workspaceSlug, projectId } = this.store.router;

const currentUserProjectRole = this.store.user.permission.projectPermissionsByWorkspaceSlugAndProjectId(
workspaceSlug?.toString() || "",
projectId?.toString() || ""
Expand All @@ -281,7 +291,6 @@ export class Page implements IPage {
*/
get canCurrentUserFavoritePage() {
const { workspaceSlug, projectId } = this.store.router;

const currentUserProjectRole = this.store.user.permission.projectPermissionsByWorkspaceSlugAndProjectId(
workspaceSlug?.toString() || "",
projectId?.toString() || ""
Expand Down
20 changes: 19 additions & 1 deletion web/core/store/pages/project-page.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import set from "lodash/set";
import unset from "lodash/unset";
import { makeObservable, observable, runInAction, action, reaction } from "mobx";
import { makeObservable, observable, runInAction, action, reaction, computed } from "mobx";
import { computedFn } from "mobx-utils";
// types
import { TPage, TPageFilters, TPageNavigationTabs } from "@plane/types";
// helpers
import { filterPagesByPageType, getPageName, orderPages, shouldFilterPage } from "@/helpers/page.helper";
// plane web constants
import { EUserPermissions } from "@/plane-web/constants";
// services
import { ProjectPageService } from "@/services/page";
// store
Expand All @@ -24,6 +26,7 @@ export interface IProjectPageStore {
filters: TPageFilters;
// computed
isAnyPageAvailable: boolean;
canCurrentUserCreatePage: boolean;
// helper actions
getCurrentProjectPageIds: (pageType: TPageNavigationTabs) => string[] | undefined;
getCurrentProjectFilteredPageIds: (pageType: TPageNavigationTabs) => string[] | undefined;
Expand Down Expand Up @@ -62,6 +65,9 @@ export class ProjectPageStore implements IProjectPageStore {
data: observable,
error: observable,
filters: observable,
// computed
isAnyPageAvailable: computed,
canCurrentUserCreatePage: computed,
// helper actions
updateFilters: action,
clearAllFilters: action,
Expand Down Expand Up @@ -92,6 +98,18 @@ export class ProjectPageStore implements IProjectPageStore {
return Object.keys(this.data).length > 0;
}

/**
* @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;
}
Comment on lines +101 to +111
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 26, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add null safety checks and improve error handling

While the implementation aligns with the PR objectives, consider these improvements for better robustness:

  1. Add null checks before toString()
  2. Handle potential undefined return from permission check
  3. 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.

Suggested change
/**
* @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 only
  • canCurrentUserArchivePage: Currently restricted to owners and admins only
  • canCurrentUserCreatePage: 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


/**
* @description get the current project page ids based on the pageType
* @param {TPageNavigationTabs} pageType
Expand Down