Skip to content

[WEB-2589] Chore: inbox issue permissions #5763

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 4 commits into from
Oct 9, 2024
Merged

[WEB-2589] Chore: inbox issue permissions #5763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6584684
chore: changed permission in inbox issue
NarayanBavisetti Oct 1, 2024
f14fd33
chore: fixed permissions for intake
gakshita Oct 7, 2024
d4b9544
fix: refactoring
gakshita Oct 8, 2024
7feafba
fix: lint
gakshita Oct 8, 2024
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
2 changes: 1 addition & 1 deletion apiserver/plane/api/views/inbox.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ def patch(self, request, slug, project_id, issue_id):
)

# Only project admins and members can edit inbox issue attributes
if project_member.role > 5:
if project_member.role > 15:
serializer = InboxIssueSerializer(
inbox_issue, data=request.data, partial=True
)
Expand Down
4 changes: 2 additions & 2 deletions apiserver/plane/app/views/inbox/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ def create(self, request, slug, project_id):
serializer.errors, status=status.HTTP_400_BAD_REQUEST
)

@allow_permission([ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST])
@allow_permission(allowed_roles=[ROLE.ADMIN], creator=True, model=Issue)
def partial_update(self, request, slug, project_id, pk):
inbox_id = Inbox.objects.filter(
workspace__slug=slug, project_id=project_id
Comment on lines +326 to 329
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 7, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect role check in access control logic

The condition in the partial_update method may not correctly enforce the intended access control. The current condition:

if project_member.role <= 5 and str(inbox_issue.created_by_id) != str(request.user.id):
    return Response(
        {"error": "You cannot edit inbox issues"},
        status=status.HTTP_400_BAD_REQUEST,
    )

Given that roles with lower numerical values indicate higher privileges (e.g., ROLE.ADMIN = 5, ROLE.MEMBER = 15, ROLE.GUEST = 20), this condition will deny access to admins who are not the creator, which might not be intended. The comment suggests that only project admins and the creator should access this endpoint.

Consider adjusting the condition to allow admins and the creator to proceed:

-if project_member.role <= 5 and str(inbox_issue.created_by_id) != str(request.user.id):
+if project_member.role > 5 and str(inbox_issue.created_by_id) != str(request.user.id):
    return Response(
        {"error": "You cannot edit inbox issues"},
        status=status.HTTP_400_BAD_REQUEST,
    )

This change ensures that users who are not admins (role > 5) and are not the creator are denied access, aligning with the intended access control logic.

Expand Down Expand Up @@ -418,7 +418,7 @@ def partial_update(self, request, slug, project_id, pk):
)

# Only project admins and members can edit inbox issue attributes
if project_member.role > 5:
if project_member.role > 15:
serializer = InboxIssueSerializer(
inbox_issue, data=request.data, partial=True
Comment on lines +421 to 423
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 7, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Role condition may incorrectly restrict member access

At line 421, the condition:

if project_member.role > 15:
    serializer = InboxIssueSerializer(
        inbox_issue, data=request.data, partial=True
    )
    # Additional logic...

This condition allows only users with a role greater than 15 (typically guests) to edit inbox issue attributes. According to the comment:

# Only project admins and members can edit inbox issue attributes

Adjust the condition to include admins and members by changing the comparison operator:

-if project_member.role > 15:
+if project_member.role <= 15:
    serializer = InboxIssueSerializer(
        inbox_issue, data=request.data, partial=True
    )

This modification ensures that users with roles of admin (role <= 5) and member (5 < role <= 15) can edit inbox issue attributes, adhering to the intended permission structure.

)
Expand Down
55 changes: 51 additions & 4 deletions web/core/components/inbox/content/inbox-issue-header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,12 @@ export const InboxIssueActionsHeader: FC<TInboxIssueActionsHeader> = observer((p
const canDelete =
allowPermissions([EUserPermissions.ADMIN], EUserPermissionsLevel.PROJECT, workspaceSlug, projectId) ||
issue?.created_by === currentUser?.id;
const isProjectAdmin = allowPermissions(
[EUserPermissions.ADMIN],
EUserPermissionsLevel.PROJECT,
workspaceSlug,
projectId
);
Comment on lines +92 to +97
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 8, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Standardize permission variables for consistency

You have introduced isProjectAdmin to specifically check for admin permissions. However, earlier in the code, permission checks use isAllowed, which includes both admin and member levels. Since the actions like accepting, declining, snoozing, and marking as duplicate are now intended for admins only, consider updating the related permission variables to use isProjectAdmin directly. This will ensure consistency and reduce the risk of permission mismatches.

const isAcceptedOrDeclined = inboxIssue?.status ? [-1, 1, 2].includes(inboxIssue.status) : undefined;
// days left for snooze
const numberOfDaysLeft = findHowManyDaysLeft(inboxIssue?.snoozed_till);
Expand Down Expand Up @@ -199,6 +205,17 @@ export const InboxIssueActionsHeader: FC<TInboxIssueActionsHeader> = observer((p
[handleInboxIssueNavigation]
);

const handleActionWithPermission = (isAdmin: boolean, action: () => void, errorMessage: string) => {
if (isAdmin) action();
else {
setToast({
type: TOAST_TYPE.ERROR,
title: "Permission denied",
message: errorMessage,
});
}
};

useEffect(() => {
if (!isNotificationEmbed) document.addEventListener("keydown", onKeyDown);
return () => {
Expand Down Expand Up @@ -293,7 +310,13 @@ export const InboxIssueActionsHeader: FC<TInboxIssueActionsHeader> = observer((p
size="sm"
prependIcon={<CircleCheck className="w-3 h-3" />}
className="text-green-500 border-0.5 border-green-500 bg-green-500/20 focus:bg-green-500/20 focus:text-green-500 hover:bg-green-500/40 bg-opacity-20"
onClick={() => setAcceptIssueModal(true)}
onClick={() =>
handleActionWithPermission(
isProjectAdmin,
() => setAcceptIssueModal(true),
"Only project admins can accept issues"
)
}
Comment on lines +313 to +319
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 8, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Synchronize button enablement with admin permissions

The "Accept" button's onClick handler correctly checks for admin permissions using isProjectAdmin. However, the button's availability is determined by canMarkAsAccepted, which is based on isAllowed and might include non-admin users. This could lead to non-admin users seeing the button enabled but encountering a permission error upon clicking. To prevent this confusion, update canMarkAsAccepted to utilize isProjectAdmin.

Apply this diff to update canMarkAsAccepted:

- const canMarkAsAccepted = isAllowed && (inboxIssue?.status === 0 || inboxIssue?.status === -2);
+ const canMarkAsAccepted = isProjectAdmin && (inboxIssue?.status === 0 || inboxIssue?.status === -2);

Committable suggestion was skipped due to low confidence.

>
Accept
</Button>
Expand All @@ -307,7 +330,13 @@ export const InboxIssueActionsHeader: FC<TInboxIssueActionsHeader> = observer((p
size="sm"
prependIcon={<CircleX className="w-3 h-3" />}
className="text-red-500 border-0.5 border-red-500 bg-red-500/20 focus:bg-red-500/20 focus:text-red-500 hover:bg-red-500/40 bg-opacity-20"
onClick={() => setDeclineIssueModal(true)}
onClick={() =>
handleActionWithPermission(
isProjectAdmin,
() => setDeclineIssueModal(true),
"Only project admins can deny issues"
)
}
Comment on lines +333 to +339
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 8, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Align "Decline" button availability with admin permissions

Similarly, the "Decline" button should reflect admin-only permissions in both its availability and action handling. Update canMarkAsDeclined to use isProjectAdmin to ensure the button is only enabled for admins, preventing non-admin users from accessing this action.

Apply this diff to update canMarkAsDeclined:

- const canMarkAsDeclined = isAllowed && (inboxIssue?.status === 0 || inboxIssue?.status === -2);
+ const canMarkAsDeclined = isProjectAdmin && (inboxIssue?.status === 0 || inboxIssue?.status === -2);

Committable suggestion was skipped due to low confidence.

>
Decline
</Button>
Expand Down Expand Up @@ -341,7 +370,15 @@ export const InboxIssueActionsHeader: FC<TInboxIssueActionsHeader> = observer((p
{isAllowed && (
<CustomMenu verticalEllipsis placement="bottom-start">
{canMarkAsAccepted && (
<CustomMenu.MenuItem onClick={handleIssueSnoozeAction}>
<CustomMenu.MenuItem
onClick={() =>
handleActionWithPermission(
isProjectAdmin,
handleIssueSnoozeAction,
"Only project admins can snooze/Un-snooze issues"
)
}
>
Comment on lines +373 to +381
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 8, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Restrict "Snooze" action to admins in the menu

The "Snooze" menu item's onClick handler checks for admin permissions using isProjectAdmin, but the menu item is displayed based on canMarkAsAccepted, which might include non-admin users. To ensure consistency and prevent non-admin users from seeing actions they cannot perform, update the condition to display the "Snooze" menu item only for admins.

Apply this diff to adjust the menu item's visibility:

- {canMarkAsAccepted && (
+ {isProjectAdmin && canMarkAsAccepted && (

Committable suggestion was skipped due to low confidence.

<div className="flex items-center gap-2">
<Clock size={14} strokeWidth={2} />
{inboxIssue?.snoozed_till && numberOfDaysLeft && numberOfDaysLeft > 0
Expand All @@ -351,7 +388,15 @@ export const InboxIssueActionsHeader: FC<TInboxIssueActionsHeader> = observer((p
</CustomMenu.MenuItem>
)}
{canMarkAsDuplicate && (
<CustomMenu.MenuItem onClick={() => setSelectDuplicateIssue(true)}>
<CustomMenu.MenuItem
onClick={() =>
handleActionWithPermission(
isProjectAdmin,
() => setSelectDuplicateIssue(true),
"Only project admins can mark issues as duplicate"
)
}
>
Comment on lines +391 to +399
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 8, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure "Mark as duplicate" action is admin-only

The "Mark as duplicate" action should be accessible only to project admins. Currently, non-admin users might see this option due to the condition based on canMarkAsDuplicate, which uses isAllowed. Update the condition to include isProjectAdmin to restrict this action appropriately.

Apply this diff to update the visibility condition:

- {canMarkAsDuplicate && (
+ {isProjectAdmin && canMarkAsDuplicate && (

Committable suggestion was skipped due to low confidence.

<div className="flex items-center gap-2">
<FileStack size={14} strokeWidth={2} />
Mark as duplicate
Expand Down Expand Up @@ -401,6 +446,8 @@ export const InboxIssueActionsHeader: FC<TInboxIssueActionsHeader> = observer((p
setIsMobileSidebar={setIsMobileSidebar}
isNotificationEmbed={isNotificationEmbed}
embedRemoveCurrentNotification={embedRemoveCurrentNotification}
isProjectAdmin={isProjectAdmin}
handleActionWithPermission={handleActionWithPermission}
/>
</div>
</>
Expand Down
44 changes: 40 additions & 4 deletions web/core/components/inbox/content/inbox-issue-mobile-header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ type Props = {
setIsMobileSidebar: (value: boolean) => void;
isNotificationEmbed: boolean;
embedRemoveCurrentNotification?: () => void;
isProjectAdmin: boolean;
handleActionWithPermission: (isAdmin: boolean, action: () => void, errorMessage: string) => void;
};

export const InboxIssueActionsMobileHeader: React.FC<Props> = observer((props) => {
Expand All @@ -70,6 +72,8 @@ export const InboxIssueActionsMobileHeader: React.FC<Props> = observer((props) =
setIsMobileSidebar,
isNotificationEmbed,
embedRemoveCurrentNotification,
isProjectAdmin,
handleActionWithPermission,
} = props;
const router = useAppRouter();
const issue = inboxIssue?.issue;
Expand Down Expand Up @@ -139,31 +143,63 @@ export const InboxIssueActionsMobileHeader: React.FC<Props> = observer((props) =
</CustomMenu.MenuItem>
)}
{canMarkAsAccepted && !isAcceptedOrDeclined && (
<CustomMenu.MenuItem onClick={handleIssueSnoozeAction}>
<CustomMenu.MenuItem
onClick={() =>
handleActionWithPermission(
isProjectAdmin,
handleIssueSnoozeAction,
"Only project admins can snooze/Un-snooze issues"
)
}
>
<div className="flex items-center gap-2">
<Clock size={14} strokeWidth={2} />
{inboxIssue?.snoozed_till && numberOfDaysLeft && numberOfDaysLeft > 0 ? "Un-snooze" : "Snooze"}
</div>
</CustomMenu.MenuItem>
)}
{canMarkAsDuplicate && !isAcceptedOrDeclined && (
<CustomMenu.MenuItem onClick={() => setSelectDuplicateIssue(true)}>
<CustomMenu.MenuItem
onClick={() =>
handleActionWithPermission(
isProjectAdmin,
() => setSelectDuplicateIssue(true),
"Only project admins can mark issues as duplicate"
)
}
>
<div className="flex items-center gap-2">
<FileStack size={14} strokeWidth={2} />
Mark as duplicate
</div>
</CustomMenu.MenuItem>
)}
{canMarkAsAccepted && (
<CustomMenu.MenuItem onClick={() => setAcceptIssueModal(true)}>
<CustomMenu.MenuItem
onClick={() =>
handleActionWithPermission(
isProjectAdmin,
() => setAcceptIssueModal(true),
"Only project admins can accept issues"
)
}
>
<div className="flex items-center gap-2 text-green-500">
<CircleCheck size={14} strokeWidth={2} />
Accept
</div>
</CustomMenu.MenuItem>
)}
{canMarkAsDeclined && (
<CustomMenu.MenuItem onClick={() => setDeclineIssueModal(true)}>
<CustomMenu.MenuItem
onClick={() =>
handleActionWithPermission(
isProjectAdmin,
() => setDeclineIssueModal(true),
"Only project admins can deny issues"
)
}
>
<div className="flex items-center gap-2 text-red-500">
<CircleX size={14} strokeWidth={2} />
Decline
Expand Down
8 changes: 4 additions & 4 deletions web/core/components/inbox/content/root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,10 @@ export const InboxContentRoot: FC<TInboxContentRoot> = observer((props) => {
}
);

const isEditable = allowPermissions(
[EUserPermissions.ADMIN, EUserPermissions.MEMBER, EUserPermissions.GUEST],
EUserPermissionsLevel.PROJECT
);
const isEditable =
allowPermissions([EUserPermissions.ADMIN], EUserPermissionsLevel.PROJECT) ||
inboxIssue.created_by === currentUser?.id;

const isGuest = projectPermissionsByWorkspaceSlugAndProjectId(workspaceSlug, projectId) === EUserPermissions.GUEST;
const isOwner = inboxIssue?.issue.created_by === currentUser?.id;
const readOnly = !isOwner && isGuest;
Expand Down
2 changes: 1 addition & 1 deletion web/core/store/inbox/project-inbox.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ export class ProjectInboxStore implements IProjectInboxStore {

if (inboxIssue && issueId) {
runInAction(() => {
set(this.inboxIssues, [issueId], new InboxIssueStore(workspaceSlug, projectId, inboxIssue, this.store));
this.createOrUpdateInboxIssue([inboxIssue], workspaceSlug, projectId);
set(this, "loader", undefined);
});
await Promise.all([
Expand Down