[WEB-2589] Chore: inbox issue permissions#5763
Conversation
|
Warning Rate limit exceeded@gakshita has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request primarily focus on enhancing user permission controls and state management for inbox issues across several components and API endpoints. Key modifications include tightening access for editing inbox issues based on user roles, refining state transition logic, standardizing error handling, and implementing permission checks in UI components. These updates ensure that only authorized users can perform specific actions related to inbox issues, thereby improving overall control flow and user feedback. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 4
🧹 Outside diff range and nitpick comments (1)
web/core/components/inbox/content/inbox-issue-header.tsx (1)
Line range hint
1-448: Overall assessment: Well-implemented permission checksThe changes in this file consistently implement permission checks for various inbox issue actions, aligning well with the PR objectives. The use of the
isProjectAdminvariable and toast notifications for feedback is a good approach. However, clarification is needed regarding the inclusion of the snooze functionality in the admin-only actions, as it wasn't explicitly mentioned in the PR objectives.Some suggestions for further improvement:
- Consider extracting the permission check and toast notification logic into a separate function to reduce code duplication.
- Update the PR description to include the snooze functionality restriction if it was intentionally added to the scope.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- apiserver/plane/api/views/inbox.py (1 hunks)
- apiserver/plane/app/views/inbox/base.py (2 hunks)
- web/core/components/inbox/content/inbox-issue-header.tsx (6 hunks)
- web/core/components/inbox/content/inbox-issue-mobile-header.tsx (4 hunks)
- web/core/components/inbox/content/root.tsx (1 hunks)
- web/core/store/inbox/project-inbox.store.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (11)
web/core/components/inbox/content/root.tsx (2)
65-67: Consider UX implications and edge casesWhile the permission changes are correctly implemented, consider the following:
- Ensure the UI clearly reflects these new permission restrictions to avoid user confusion.
- Update any relevant documentation or user guides to explain the new permission model.
- Consider adding a comment explaining the permission logic for future maintainability.
To address point 1, let's check if there are any UI components that need updating:
This will help identify any UI components that might need to be updated to reflect the new permission model.
65-67: LGTM: Permission logic updated as per requirements.The changes to the
isEditablelogic correctly implement the new permission requirements. Only admin users or the creator of the inbox issue can now edit it, which aligns with the PR objectives.To ensure consistency, let's verify if similar permission checks are implemented elsewhere in the codebase:
This will help us identify if the same permission logic is applied consistently across the application.
✅ Verification successful
To address the errors in the previous script, here's a corrected version to search for similar permission checks:
To successfully search for similar permission checks, please run the following corrected script:
Verified: Permission logic is consistent across the codebase.
The changes to the
isEditablelogic align with existing permission checks throughout the codebase, ensuring only admin users or the issue creator can edit inbox issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for similar permission checks in other files # Test: Look for similar permission checks rg --type typescript --type javascript -e "allowPermissions\(\[.*ADMIN.*\].*\)" -e "created_by.*===.*currentUser.*id"Length of output: 213
Script:
#!/bin/bash # Description: Search for similar permission checks in other files # Test: Look for similar permission checks rg --type=typescript --type=javascript -e "allowPermissions\\(\\[.*ADMIN.*\\].*\\)" -e "created_by\\s*===\\s*currentUser\\s*\\.id"Length of output: 159
Script:
#!/bin/bash # Description: Search for similar permission checks in TypeScript and JavaScript files # Find all .ts and .tsx files and search for the permission patterns fd -e ts -e tsx -e js -e jsx | xargs rg -e "allowPermissions\(\[.*ADMIN.*\].*\)" -e "created_by\s*===\s*currentUser\s*\.id"Length of output: 7117
web/core/components/inbox/content/inbox-issue-header.tsx (5)
92-97: LGTM: Addition ofisProjectAdminvariableThe introduction of the
isProjectAdminvariable is a good approach to centralize the admin permission check. This aligns well with the PR objectives of restricting certain actions to admin users only.
302-310: LGTM: Permission check for accepting issuesThe implementation of the permission check for accepting issues is correct and aligns with the PR objectives. The use of a toast notification for non-admin users provides clear feedback, enhancing the user experience.
324-332: LGTM: Permission check for declining issuesThe implementation of the permission check for declining issues is correct and consistent with the changes made for accepting issues. This ensures that only admin users can decline issues, as specified in the PR objectives.
386-396: LGTM: Permission check for marking issues as duplicatesThe implementation of the permission check for marking issues as duplicates is correct and aligns with the PR objectives. This ensures that only admin users can perform this action, as specified. The consistent use of toast notifications for feedback is a good practice.
366-376: LGTM with a question: Permission check for snoozing issuesThe implementation of the permission check for snoozing issues is consistent with other changes and provides appropriate user feedback. However, the PR objectives didn't explicitly mention restricting the snooze functionality to admin users. Could you clarify if this was an intentional addition to the scope of the PR?
web/core/store/inbox/project-inbox.store.ts (1)
426-426: Excellent refactoring to improve code maintainabilityThe change to use
this.createOrUpdateInboxIssue([inboxIssue], workspaceSlug, projectId)instead of directly assigning a newInboxIssueStoreinstance is a great improvement. This refactoring:
- Consolidates the logic for creating or updating inbox issues into a single method.
- Reduces code duplication, adhering to the DRY principle.
- Provides a consistent interface for handling both single and multiple issues.
These changes enhance the overall maintainability and readability of the codebase.
web/core/components/inbox/content/inbox-issue-mobile-header.tsx (2)
18-18: Import ofTOAST_TYPEandsetToastis appropriateThe addition of
TOAST_TYPEandsetToastfrom@plane/uienables the display of toast notifications, which is necessary for informing users about permission issues.
50-50: Addition ofisProjectAdminprop for permission checksIntroducing the
isProjectAdmin: booleanprop and destructuring it frompropsis appropriate. This allows the component to conditionally render actions based on the user's admin status.Also applies to: 74-74
apiserver/plane/api/views/inbox.py (1)
Line range hint
288-332: Ensure role permissions accurately reflect admin accessThe condition
if project_member.role > 15:is intended to allow only admin users to edit inbox issue attributes. However, depending on how roles are defined in your system, this comparison may not correctly restrict access to admins.Assuming role values are:
- Guest: 5
- Member: 15
- Admin: 20
Using
project_member.role > 15would allow roles with values 16 and above to have access, which might include roles that are not admins. To ensure that only admins can edit inbox issues, consider updating the condition to check for admin roles explicitly.Apply this diff to correct the role check:
-if project_member.role > 15: +if project_member.role >= 20:Run the following script to confirm the role values defined for
ProjectMember:✅ Verification successful
Role permission check is correctly implemented
The condition
if project_member.role > 15:appropriately restricts access to only admin users based on the defined role values.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the numerical values assigned to different roles. # Expected: Find definitions or mappings of role values to role names. rg --type python 'ROLE_' -A 2 -B 2Length of output: 65
Script:
#!/bin/bash # Description: Verify the numerical values assigned to different roles by searching for ROLE_ definitions in Python files. # Expected: Find definitions or mappings of role values to role names. rg 'ROLE_' --glob '*.py' -A 2 -B 2Length of output: 2980
| type: TOAST_TYPE.ERROR, | ||
| title: "Permission denied", | ||
| message: "Only project admins can deny issues", | ||
| }) |
There was a problem hiding this comment.
Ensure consistent terminology: Use "decline" instead of "deny"
In the error message on line 205, the term "deny" is used, whereas the corresponding action is labeled "Decline" elsewhere in the code and UI. For consistency and to avoid confusion, replace "deny" with "decline" in the error message.
Apply this diff to correct the terminology:
type: TOAST_TYPE.ERROR,
title: "Permission denied",
- message: "Only project admins can deny issues",
+ message: "Only project admins can decline issues",
})📝 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.
| type: TOAST_TYPE.ERROR, | |
| title: "Permission denied", | |
| message: "Only project admins can deny issues", | |
| }) | |
| type: TOAST_TYPE.ERROR, | |
| title: "Permission denied", | |
| message: "Only project admins can decline issues", | |
| }) |
| @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 |
There was a problem hiding this comment.
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.
| if project_member.role > 15: | ||
| serializer = InboxIssueSerializer( | ||
| inbox_issue, data=request.data, partial=True |
There was a problem hiding this comment.
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 attributesAdjust 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.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
web/core/components/inbox/content/inbox-issue-mobile-header.tsx (1)
151-152: Standardize terminology: Use "Unsnooze" without a hyphenConsider using "Unsnooze" instead of "Un-snooze" to maintain consistency and adhere to standard spelling conventions.
web/core/components/inbox/content/inbox-issue-header.tsx (1)
208-218: RenamehandleActionWithPermissionfor clarityThe function
handleActionWithPermissionexecutes an action based on admin permissions. To enhance readability and adhere to naming conventions, consider renaming it to something more descriptive, such asexecuteIfAdminorexecuteWithAdminCheck.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- web/core/components/inbox/content/inbox-issue-header.tsx (7 hunks)
- web/core/components/inbox/content/inbox-issue-mobile-header.tsx (4 hunks)
🧰 Additional context used
🔇 Additional comments (4)
web/core/components/inbox/content/inbox-issue-mobile-header.tsx (3)
146-154: Good use of helper function to reduce code duplicationReplacing repeated permission checks with
handleActionWithPermissionenhances code readability and adheres to the DRY principle.
162-170: Consistent application ofhandleActionWithPermissionUtilizing the helper function here maintains consistency across permission checks and improves maintainability.
178-186: Consistent application ofhandleActionWithPermissionUsing the helper function for permission checks enhances code clarity and adheres to best practices.
web/core/components/inbox/content/inbox-issue-header.tsx (1)
449-450: Verify mobile header enforces admin permissionsBy passing
isProjectAdminandhandleActionWithPermissiontoInboxIssueActionsMobileHeader, you're aiming to enforce permission checks in the mobile view. Ensure that this component correctly utilizes these props to restrict admin-only actions and hides or disables them for non-admin users.Run the following script to verify the usage:
Expected Result:
The output should show that
InboxIssueActionsMobileHeaderusesisProjectAdminandhandleActionWithPermissionto enforce admin-only actions appropriately.
| const isProjectAdmin = allowPermissions( | ||
| [EUserPermissions.ADMIN], | ||
| EUserPermissionsLevel.PROJECT, | ||
| workspaceSlug, | ||
| projectId | ||
| ); |
There was a problem hiding this comment.
🛠️ 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.
| onClick={() => | ||
| handleActionWithPermission( | ||
| isProjectAdmin, | ||
| () => setAcceptIssueModal(true), | ||
| "Only project admins can accept issues" | ||
| ) | ||
| } |
There was a problem hiding this comment.
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.
| <CustomMenu.MenuItem | ||
| onClick={() => | ||
| handleActionWithPermission( | ||
| isProjectAdmin, | ||
| () => setSelectDuplicateIssue(true), | ||
| "Only project admins can mark issues as duplicate" | ||
| ) | ||
| } | ||
| > |
There was a problem hiding this comment.
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.
| <CustomMenu.MenuItem | ||
| onClick={() => | ||
| handleActionWithPermission( | ||
| isProjectAdmin, | ||
| handleIssueSnoozeAction, | ||
| "Only project admins can snooze/Un-snooze issues" | ||
| ) | ||
| } | ||
| > |
There was a problem hiding this comment.
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.
| onClick={() => | ||
| handleActionWithPermission( | ||
| isProjectAdmin, | ||
| () => setDeclineIssueModal(true), | ||
| "Only project admins can deny issues" | ||
| ) | ||
| } |
There was a problem hiding this comment.
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.
* chore: changed permission in inbox issue * chore: fixed permissions for intake * fix: refactoring * fix: lint --------- Co-authored-by: NarayanBavisetti <narayan3119@gmail.com>
5 participants
Summary
[WEB-2589]
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
These updates ensure a more secure and user-friendly experience when managing inbox issues.