Skip to content

feat: tool import permission #7797

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: tool import permission #7797

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
15 changes: 14 additions & 1 deletion ui/src/views/tool/components/ButtonCreateTool.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,19 @@ import McpFormDrawer from '../tool-form/McpFormDrawer.vue'
import SkillToolFormDrawer from '../tool-form/SkillToolFormDrawer.vue'
import WorkflowFormDialog from '../tool-form/WorkflowFormDialog.vue'
import perm from '@/permission/index.ts'
import { isSystemResource, isSystemSharedResource, isWorkspaceSharedResource } from '@/utils/resource-context'

/**
* 工具「导入」权限判断
*/
type ToolImportPermissionMethod = 'import'

function toolImportPermission(method: ToolImportPermissionMethod): boolean {
if (isSystemResource()) return Boolean(perm.tool.system[method]())
if (isSystemSharedResource()) return Boolean(perm.tool.share[method]())
if (isWorkspaceSharedResource()) return Boolean(perm.tool.workspaceShared[method]())
return Boolean(perm.tool.workspace[method]())
}

defineOptions({ name: 'ButtonCreateTool' })

Expand Down Expand Up @@ -170,7 +183,7 @@ function handleRefresh() {
multiple
:on-change="handleImportCreate"
:show-file-list="false"
v-if="perm.tool.workspace.import()"
v-if="toolImportPermission('import')"
>
<MkDropdownItem class="w-full py-2!">
<template #icon>

@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.

Here is the review of the provided code snippet, focusing on logic, type safety, and Vue best practices.

1. Logical & Safety Issues

Potential Runtime Error: Unsafe Property Access

The code assumes that perm.tool.system, perm.tool.share, and perm.tool.workspaceShared always exist and have an import method.

if (isSystemResource()) return Boolean(perm.tool.system[method]())
  • Risk: If perm.tool.system is undefined or missing the method for a specific tool type, this will throw a TypeError.
  • Suggestion: Add optional chaining or fallbacks. 
    if (isSystemResource()) return Boolean(perm?.tool?.system?.[method]?.())

Type Safety with Computed Keys

While ToolImportPermissionMethod restricts method to 'import', accessing perm.tool.system[method] dynamically bypasses TypeScript's strict property checking unless perm is typed as an any or a very loose record.

  • Suggestion: Explicitly cast or define the shape of perm to ensure the methods exist. Alternatively, since there is only one method ('import'), consider removing the parameter and hardcoding it to simplify the logic and avoid dynamic key lookup overhead/misuse.

2. Optimization Suggestions

Redundant Boolean() Wrapper

Permission functions typically return boolean. Wrapping the result in Boolean() is redundant unless you suspect they return non-boolean truthy/falsy values (like strings or numbers). If perm.tool.*.import() returns true/false or undefined, simply returning the value handles falsy checks correctly in JavaScript/TS.

  • Suggestion: 
    // Instead of: return Boolean(perm.tool.workspaceShared[method]())
return !!perm.tool.workspaceShared?.[method]() // Or just return the value if strictly boolean

Function Name & Caching

The function toolImportPermission is called in the template via v-if. Vue re-evaluates v-if expressions reactively. Since isSystemResource(), etc., are likely reactive or state-dependent, this is fine. However, if perm is static, you could compute this once. But given typical Vue patterns, calling the function is acceptable.

3. Code Style & Best Practices

Use Computed Property Instead of Function Call in Template (Optional)

If the permissions do not change frequently after component mount, using a computed property can be cleaner than calling a function in the template.

<script setup lang="ts">
// ... imports ...

const canImport = computed(() => toolImportPermission('import'))
</script>

<template>
  <!-- Use v-if="canImport" -->
</template>

This separates logic from presentation slightly and makes the template cleaner.

Naming Convention

ToolImportPermissionMethod is verbose. Since it only represents 'import', consider simplifying:

type PermissionAction = 'import' // Future-proof if more actions are added

4. Refactored Code Example

Here is a safer, cleaner version:

import { isSystemResource, isSystemSharedResource, isWorkspaceSharedResource } from '@/utils/resource-context'
import perm from '@/permission/index.ts'

/**
 * Checks import permission based on resource context.
 */
function checkImportPermission(): boolean {
  // Use optional chaining to prevent crashes if perm structure varies
  if (isSystemResource()) return !!perm.tool.system?.import?.()
  if (isSystemSharedResource()) return !!perm.tool.share?.import?.()
  if (isWorkspaceSharedResource()) return !!perm.tool.workspaceShared?.import?.()
  
  return !!perm.tool.workspace?.import?.()
}

// Define as computed if used in template (recommended):
// const canImportTool = computed(checkImportPermission)

In the template:

<!-- If using function directly -->
v-if="checkImportPermission()"

<!-- Or if using computed (preferred) -->
v-if="canImportTool"

Summary of Recommendations:

  1. Add Optional Chaining (?.) to all permission method calls to prevent runtime crashes.
  2. Remove Boolean() wrapper if permissions return proper booleans; use !! if forcing boolean coercion is needed.
  3. Consider computed for better template readability and performance caching.
  4. Hardcode 'import' if no other methods are planned immediately, to reduce complexity.

Expand Down
Loading