Skip to content

feat: workspace tool edit permission #7781

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
peslyn merged 1 commit into from
Nov 10, 2026
Merged

feat: workspace tool edit permission #7781

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
4 changes: 2 additions & 2 deletions ui/src/views/tool/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ const router = useRouter()
const editToolFormRef = useTemplateRef<InstanceType<typeof EditToolForm>>('editToolFormRef')

function handleOpenEditTool(tool: ToolItem) {
if (!perm.tool.workspace.edit(tool.id)) return
return editToolFormRef.value?.open(tool)
if (!perm.tool.workspace.read()) return
return editToolFormRef.value?.open(tool)
}

function handleOpenTool(tool: ToolItem, event: MouseEvent) {

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Code Review

Issues Found

1. Permission regression: edit → read (likely a bug)

-  if (!perm.tool.workspace.edit(tool.id)) return
+  if (!perm.tool.workspace.read()) return

The function is named handleOpenEditTool and opens an edit form (editToolFormRef). Gatekeeping it with read() instead of edit(tool.id) means:

  • Users with only read permission can now open the edit form, which is a privilege escalation / security concern.
  • The signature changed from edit(tool.id) (per-tool check) to read() (no tool argument). If read() is workspace-wide, you lose per-tool granularity.

Recommendation: Keep perm.tool.workspace.edit(tool.id). If a read-then-edit flow is intentional, rename the function or split into separate open/view handlers.

2. Inconsistent indentation

+return editToolFormRef.value?.open(tool)

The +return line lost its two-space indentation relative to the function body. This will fail lint/prettier checks in most setups.

Fix:

function handleOpenEditTool(tool: ToolItem) {
  if (!perm.tool.workspace.read()) return
  return editToolFormRef.value?.open(tool)
}

3. API signature mismatch on read()

edit(tool.id) accepts an argument; read() is called with none. Confirm the permission API actually exposes read() (vs. e.g. read(tool.id)), otherwise this throws or silently returns undefined.

4. Silent failure — no user feedback

When the permission check fails, the function silently returns. Consider surfacing a toast/notification ("No permission to edit this tool") so users understand why nothing happened. Same issue pre-existed, but it's worth addressing while touching this code.

5. return type inconsistency

The first branch returns implicitly undefined; the second returns the result of ?.open(...) (boolean | undefined or whatever open returns). Not wrong, but if callers ever rely on the return value, define an explicit return type (e.g. : void) to avoid surprises.

Suggested Refactor

function handleOpenEditTool(tool: ToolItem): void {
  if (!perm.tool.workspace.edit(tool.id)) {
    // optionally: toast.error('No permission to edit this tool')
    return
  }
  editToolFormRef.value?.open(tool)
}

Bottom line: The diff introduces what looks like an unintended permission downgrade and a formatting glitch. Verify whether switching from edit(tool.id) to read() was intentional — if not, revert it.

Expand Down
12 changes: 12 additions & 0 deletions ui/src/views/tool/tool-form/EditToolForm.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,17 @@ import DataSourceFormDrawer from '@/views/tool/tool-form/DataSourceFormDrawer.vu
import McpFormDrawer from '@/views/tool/tool-form/McpFormDrawer.vue'
import SkillToolFormDrawer from '@/views/tool/tool-form/SkillToolFormDrawer.vue'
import WorkflowFormDialog from '@/views/tool/tool-form/WorkflowFormDialog.vue'
import { computed } from 'vue'
import perm from '@/permission/index.ts'
import { isSystemResource, isSystemSharedResource } from '@/utils/resource-context'

const canEdit = computed(() => {
const id = tool.value?.id
if (!id) return false
if (isSystemResource()) return perm.tool.system.edit()
if (isSystemSharedResource()) return perm.tool.share.edit()
return perm.tool.workspace.edit(id)
})

defineOptions({ name: 'EditToolForm' })

Expand Down Expand Up @@ -95,6 +106,7 @@ defineExpose({ open })
:folder-id="tool.folder_id ?? ''"
@closed="handleFormClosed"
@update="emit('update', $event)"
:can-edit="canEdit"
/>
</template>
</template>

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review

Potential issues

  1. computed used before tool is declared – The diff shows canEdit defined at the top of <script setup> referencing tool.value, but tool isn't visible in the snippet. If tool is declared after this block, it will throw a ReferenceError (TDZ) at module init when the getter runs. Ensure tool (likely a ref) is declared before canEdit.

  2. computed return passed as a ref-like prop – In the template you pass :can-edit="canEdit". Vue auto-unwraps refs in templates, so canEdit becomes a boolean value — that's fine. But if DataSourceFormDrawer (or whichever drawer receives can-edit) expects a plain Boolean and uses props.canEdit directly, it will work. Just verify the receiving component defines canEdit: Boolean and not e.g. compares against a ref. If children receive it via v-bind="$attrs" further down, unwrapping still happens at each template boundary, so ok.

  3. Non-reactive inputs to canEdit – isSystemResource() and isSystemSharedResource() look like global/context helpers. computed only re-evaluates when reactive deps change (tool.value?.id). If the "system vs shared vs workspace" context can change while the drawer stays mounted, canEdit will be stale. Verify whether those helpers read reactive state (e.g., route params via useRoute()) — if not, the computed won't recompute.

  4. Import order / style – New imports are appended after existing ones rather than sorted; perm import from '@/permission/index.ts' includes the .ts extension whereas most projects omit it. Minor consistency issue.

  5. Duplicated permission branching – This pattern (system.edit / share.edit / workspace.edit(id)) likely exists elsewhere. Consider extracting into a shared helper (e.g., canEditTool(tool)) to avoid drift.

  6. Falsy id handling – if (!id) return false treats id === 0 or '' as missing. Probably fine for UUID-style ids, but note it.

  7. Sample data operation attribution is ambiguous in the annotation header – the hunk header says @@ -10,6 +10,17 @@, i.e. 11 added lines, and the block matches exactly. Just double-check nothing was dropped between the two hunks (lines 20–106 aren't shown).

Optimization suggestions

  • Move the import additions above the existing ones or rely on the project's import-sorting lint rule.
  • Compute once with early returns as done — good. Optionally memoize the perm calls if they're expensive, though typically trivial.
  • Add a unit test asserting canEdit for the three tenant contexts, since permission logic is easy to regress.
  • If perm.tool.*.edit() throws for undefined args, ensure id truthiness check precedes the workspace.edit(id) call (it does).

Verdict

Functionally looks correct assuming tool is declared earlier and the permission-context helpers are static per mount. The main real risk is the declaration-order / reactivity-of-context concern (items 1 & 3).

9 changes: 6 additions & 3 deletions ui/src/views/tool/tool-form/tool-custom/ToolFormDrawer.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ defineOptions({ name: 'ToolFormDrawer' })

const { auth } = useStore()

const props = defineProps<{ api: typeof ToolApi | typeof SystemSharedToolApi | typeof SystemResourceToolApi; title: string; folderId: string }>()
const props = withDefaults(
defineProps<{ api: typeof ToolApi | typeof SystemSharedToolApi | typeof SystemResourceToolApi; title: string; folderId: string; canEdit?: boolean }>(),
{ canEdit: true },
)

const emit = defineEmits<{ closed: []; refresh: []; update: [tool: ToolItem] }>()

Expand Down Expand Up @@ -228,8 +231,8 @@ defineExpose({ open })

<template #footer>
<el-button plain :disabled="loading || formLoading" @click="handleBeforeClose">取消</el-button>
<el-button plain :disabled="loading || formLoading" @click="handleOpenDebug">调试</el-button>
<el-button type="primary" :disabled="formLoading" :loading="loading" @click="handleSubmit">
<el-button v-if="canEdit" plain :disabled="loading || formLoading" @click="handleOpenDebug">调试</el-button>
<el-button v-if="canEdit" type="primary" :disabled="formLoading" :loading="loading" @click="handleSubmit">
{{ editId ? '保存' : '创建' }}
</el-button>
</template>

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review

1. Prop name canEdit is misleading

The prop hides the debug button as well as the submit button. "Debug" is not an edit action — a read-only viewer would still reasonably expect to debug a tool. Consider naming it readonly (inverted, matching Vue/Element Plus convention) or splitting into two props (canSubmit, canDebug).

2. withDefaults + optional boolean — fine, but be explicit

defineProps<{ ...; canEdit?: boolean }>()

With { canEdit: true } this works. Note that booleans in Vue default to false when absent only if no default is set; since you set it, behavior is true by omission. That's intentional here, but callers who pass <ToolFormDrawer :can-edit="undefined"> will get true — usually acceptable.

3. Template uses canEdit directly instead of props.canEdit

In <script setup> this resolves correctly, but for consistency with existing code that uses loading, formLoading etc. it's fine — just keep the style uniform across the file.

4. Missing v-if on the cancel button — inconsistent UX

The layout now shows one button in read-only mode ("取消"). Verify the drawer footer looks OK alone. If the intent is a pure detail view, consider whether "取消" should become "关闭".

5. Guard submit logic in script, not only in template

Templated v-if is UI only. If handleSubmit / handleOpenDebug can be triggered elsewhere (keyboard shortcut, exposed method via defineExpose, watcher), add a guard:

const handleSubmit = () => {
  if (!props.canEdit) return
  ...
}

Same for handleOpenDebug.

6. api prop type is a union of three typeof XxxApi

None of the changes touch this, but worth noting: a union of API module types means every method usage must be compatible with all three. If the APIs differ slightly, this weakens type safety — prefer a shared interface ToolApiLike that all three satisfy.

7. Diff indentation

The diff fragments show leading spaces inside the hunk ( @@ ... and <template #footer>). Likely just formatting from pasting; ensure actual source has consistent indentation so lint/prettier don't flag it.

8. Tests / stories

If ToolFormDrawer has unit tests or Storybook stories, add a case for canEdit: false to lock the read-only behavior (buttons hidden, submit methods no-op after the guard in #5).

Summary

The change is minimal and correct. Main recommendations: rename/split the prop for clarity (#1), add script-level guards (#5), and align the read-only footer UX (#4).

Expand Down
Loading