fix(web): respect granular task permissions - #1520
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR replaces aggregate task and label permissions with operation-specific checks. It updates task controls, bulk toolbars, workspace label settings, task descriptions, subtasks, and task-label mutations. ChangesGranular permissions and label actions
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TaskLabelsPopover
participant LabelMutation
participant LabelAPI
TaskLabelsPopover->>LabelMutation: attach or detach task label
LabelMutation->>LabelAPI: send label-task request
LabelAPI-->>LabelMutation: return response
LabelMutation-->>TaskLabelsPopover: update cached labels
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
PR Summary by QodoFix web UI to honor granular task/label permissions
AI Description
Diagram
High-Level Assessment
Files changed (25)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/components/task/task-description.tsx`:
- Around line 267-268: Use the canEdit value from useWorkspacePermission to gate
all task-description editor mutations: guard language-selector and BubbleMenu
editor.chain actions, and prevent onUpdate from publishing changes when canEdit
is false. Keep editor.setEditable(canEdit) and preserve read-only actions such
as code copying.
In `@apps/web/src/components/task/task-subtasks.tsx`:
- Around line 71-72: In the task-subtask component, read canCreateTasks()
separately from canUpdateTasks() and use the resulting canCreate permission for
the add-subtask controls and handleAddSubtask gating. Keep canEdit backed by
canUpdateTasks() for status and other edit controls, and ensure creation is
allowed only when task-create permission is present.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eddac81c-58e5-4dc1-a30c-e347e2a3c16d
📒 Files selected for processing (25)
apps/web/src/components/bulk-selection/backlog-bulk-toolbar.tsxapps/web/src/components/bulk-selection/bulk-toolbar.tsxapps/web/src/components/kanban-board/column/column-header.tsxapps/web/src/components/kanban-board/task-card-context-menu/task-card-context-menu-content.tsxapps/web/src/components/shared/modals/create-task-modal.test.tsxapps/web/src/components/shared/modals/create-task-modal.tsxapps/web/src/components/task/subtask-status-popover.tsxapps/web/src/components/task/task-description.tsxapps/web/src/components/task/task-due-date-popover.tsxapps/web/src/components/task/task-labels-popover.tsxapps/web/src/components/task/task-priority-popover.tsxapps/web/src/components/task/task-relations.tsxapps/web/src/components/task/task-start-date-popover.tsxapps/web/src/components/task/task-status-popover.test.tsxapps/web/src/components/task/task-status-popover.tsxapps/web/src/components/task/task-subtasks.test.tsxapps/web/src/components/task/task-subtasks.tsxapps/web/src/components/task/task-title.tsxapps/web/src/fetchers/label/attach-label-to-task.tsapps/web/src/fetchers/label/detach-label-from-task.tsapps/web/src/hooks/mutations/label/use-attach-label-to-task.tsapps/web/src/hooks/mutations/label/use-detach-label-from-task.tsapps/web/src/hooks/use-workspace-permission.test.tsxapps/web/src/hooks/use-workspace-permission.tsapps/web/src/routes/_layout/_authenticated/dashboard/settings/workspace/labels.tsx
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64f9f560b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await attachLabel({ | ||
| labelId: workspaceLabel.id, | ||
| taskId: task.id, | ||
| workspaceId, | ||
| }); |
There was a problem hiding this comment.
When the label list contains only a task-scoped copy—such as a GitHub/Gitea-imported label without a workspace-level row—workspaceLabel.id identifies the copy already assigned to another task. assignLabelToTask deletes that row from its previous task before inserting it for the target, so selecting the label here silently moves it instead of copying it as the previous implementation did. Only pass workspace-level labels to this endpoint, or retain copy semantics for task-scoped rows.
Useful? React with 👍 / 👎.
| const { canUpdateTasks } = useWorkspacePermission(); | ||
| const canEdit = canUpdateTasks(); |
There was a problem hiding this comment.
For a role with task:update but not task:create, this makes canEdit true and renders the Add Subtask control, but handleAddSubtask calls the task-creation endpoint, which requires task:create, so every attempt ends in an authorization error. Subtask creation should be gated by both the update permission needed to create the relation and the create permission needed to create the task.
Useful? React with 👍 / 👎.
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/components/task/task-description.tsx`:
- Line 757: Update the debouncedUpdate callback in the task description editor
to recheck canEditRef.current immediately before calling updateTaskDescription,
while preserving the existing scheduling guard and syncing-content behavior.
In `@apps/web/src/components/task/task-subtasks.tsx`:
- Line 262: Use the task-create permission alone for subtask creation: in
apps/web/src/components/task/task-subtasks.tsx lines 262-262, remove canEdit
from the creation guard; at lines 341-341 and 402-402, conditionally render the
add button and creation form when canCreate is true. In
apps/web/src/components/task/task-subtasks.test.tsx lines 162-179, add coverage
for task-create allowed with task-update denied and verify the add action
remains available.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d505b75-16c0-49d4-9a65-f1b0ea306b90
📒 Files selected for processing (6)
apps/web/src/components/task/task-description.tsxapps/web/src/components/task/task-labels-popover.tsxapps/web/src/components/task/task-subtasks.test.tsxapps/web/src/components/task/task-subtasks.tsxapps/web/src/lib/get-task-label-options.test.tsapps/web/src/lib/get-task-label-options.ts
|
|
||
| const handleAddSubtask = async () => { | ||
| if (!newTitle.trim()) return; | ||
| if (!canCreate || !canEdit || !newTitle.trim()) return; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use task-create permission alone for subtask creation.
The add-subtask flow now requires canUpdateTasks() as well as canCreateTasks(). This denies task-create-only members, even though handleAddSubtask creates a task.
apps/web/src/components/task/task-subtasks.tsx#L262-L262: RemovecanEditfrom the creation guard.apps/web/src/components/task/task-subtasks.tsx#L341-L341: Render the add button whencanCreateis true.apps/web/src/components/task/task-subtasks.tsx#L402-L402: Render the creation form whencanCreateis true.apps/web/src/components/task/task-subtasks.test.tsx#L162-L179: Add a case with task-create allowed and task-update denied. Verify that the add action remains available.
Proposed source fix
- if (!canCreate || !canEdit || !newTitle.trim()) return;
+ if (!canCreate || !newTitle.trim()) return;
- {canEdit && canCreate && (
+ {canCreate && (
- {isAdding && canEdit && canCreate && (
+ {isAdding && canCreate && (📍 Affects 2 files
apps/web/src/components/task/task-subtasks.tsx#L262-L262(this comment)apps/web/src/components/task/task-subtasks.tsx#L341-L341apps/web/src/components/task/task-subtasks.tsx#L402-L402apps/web/src/components/task/task-subtasks.test.tsx#L162-L179
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/components/task/task-subtasks.tsx` at line 262, Use the
task-create permission alone for subtask creation: in
apps/web/src/components/task/task-subtasks.tsx lines 262-262, remove canEdit
from the creation guard; at lines 341-341 and 402-402, conditionally render the
add button and creation form when canCreate is true. In
apps/web/src/components/task/task-subtasks.test.tsx lines 162-179, add coverage
for task-create allowed with task-update denied and verify the add action
remains available.
luantaraschi
left a comment
luantaraschi
left a comment
There was a problem hiding this comment.
Checked this out at 5edd50dd, alongside #1559, which lands on the same issue. Since two PRs are open against #1505 I measured how they relate rather than reviewing this one in isolation.
The core of the fix is byte-identical in both PRs. Nine of the seventeen files #1559 touches are the same content at both heads, and each is a two-line change in #1559:
column-header.tsx, subtask-status-popover.tsx, task-due-date-popover.tsx,
task-priority-popover.tsx, task-relations.tsx, task-start-date-popover.tsx,
task-status-popover.tsx, task-status-popover.test.tsx, task-title.tsx
Both split manageTasks: { task: ["create","update","delete"] } into updateTasks and deleteTasks, with the same capability names and the same helper names. So on the task half of #1505 there is no disagreement between the two, and #1559's seventeen files are a strict subset of this PR's twenty-seven.
The difference is the label half, and #1505 asks for it explicitly. The report lists four broken things: state, dates, labels, priorities. #1559 stops at manageLabels, which still requires label: ["create","update","delete"] together:
// #1559, task-labels-popover.tsx
const { canUpdateTasks, canManageLabels } = useWorkspacePermission();
const canCreateLabels = canManageLabels();The reporter's role grants labels create, read and update, with no delete, which is exactly the combination that fails that check. So #1559 fixes three of the four symptoms for that role and leaves the label one where it was. This PR splits the label capability into create, update and delete, which is what closes the fourth.
The label gating here matches the server, and the comment it removes did not. I went looking for a problem in gating attach and detach on canUpdateLabels, because the comment on main says attaching a label to a task is a task mutation. The API disagrees with that comment. Both routes in apps/api/src/label/index.ts guard on the label capability:
.put("/:id/task", ..., requireWorkspacePermission({ label: ["update"] }), ...)
.delete("/:id/task", ..., requireWorkspacePermission({ label: ["update"] }), ...)So this PR's canUpdateLabels() mirrors what the server will actually enforce, and gating on task update would have shown controls that then fail with a 403. The new fetchers also target routes that already exist on main, so nothing here waits on an API change.
What I ran, on Windows with Node 22.20.0 and pnpm 10.34.1:
| tree | test files | tests |
|---|---|---|
main 3fc77f00 |
27 passed | 82 passed |
#1520 5edd50dd |
26 passed | 75 passed |
#1559 a588f776 |
24 passed | 71 passed |
Each tree also has the same three failures, and all three are mine rather than anyone's code: two files import @kaneo/permissions, which my install did not build, and src/env.test.ts shells out to sed, which Windows does not have. The counts differ because the branches sit on different bases, not because tests were removed.
For whoever decides: this PR is the superset and the one that closes #1505 as filed. #1559 is a clean, smaller change that agrees with it line for line on the task half, so if you would rather land the small one first, the two do not conflict in intent, only in size. What I would avoid is merging #1559 and treating #1505 as done, since the label case that the reporter described would still be broken.
Thanks to both of you for working the same bug carefully.
#1581 added this test while the editor still read canManageTasks, and #1520 renamed that capability to canUpdateTasks. Each PR was green on its own branch; merged together the mock returns an object without the function the component destructures, so the render throws. Nothing to fix in either change, only the mock they were both written against.
3 participants
Description
Respect task and label permissions individually in the task editing UI. Members with task update permission can edit task fields without requiring task delete permission.
Related Issue(s)
Fixes #1505
Type of Change
How Has This Been Tested?
Screenshots (if applicable)
Checklist
Additional Notes
Summary by CodeRabbit
New Features
Bug Fixes