Skip to content

fix: knowledge problem permission #7752

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 9, 2026
Merged

fix: knowledge problem permission #7752

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
16 changes: 15 additions & 1 deletion ui/src/views/knowledge-detail/problem/components/ProblemDetailDrawer.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,20 @@ import { getFileIconUrl } from '@/utils/icon'
import { getFileUrl } from '@/utils/common'
import { DOCUMENT_RESOURCE_TYPE } from '@/api/enums'
import { KNOWLEDGE_TYPE_KEY, KNOWLEDGE_TYPE_MAP } from '@/constants/knowledge'
import { isWorkspaceSharedResource,isSystemResource, isSystemSharedResource } from '@/utils/resource-context'
import { perm } from '@/permission'

/**
* 知识库「问题」权限判断
*/
type KnowledgeProblemPermissionMethod = 'problemRelate'

function knowledgeProblemPermission(method: KnowledgeProblemPermissionMethod): boolean {
if (isSystemResource()) return Boolean(perm.knowledge.system[method]())
if (isSystemSharedResource()) return Boolean(perm.knowledge.share[method]())
if (isWorkspaceSharedResource()) return false // 共享知识库在工作空间侧只读
return Boolean(perm.knowledge.workspace[method](knowledgeId.value))
}

defineOptions({ name: 'ProblemDetailDrawer' })
const problem = defineModel<ProblemItem>({ required: true })
Expand Down Expand Up @@ -161,7 +175,7 @@ defineExpose({ refresh: handleLoadParagraphs })
</div>
<template #footer>
<!-- 关联当前问题的分段 -->
<el-button plain :disabled="loading" @click="emit('associate')">关联分段</el-button>
<el-button plain :disabled="loading" v-if="knowledgeProblemPermission('problemRelate')" @click="emit('associate')">关联分段</el-button>
<!-- 查看上一个问题 -->
<el-button plain :disabled="loading || !hasPrevious" @click="emit('previous')">上一个</el-button>
<!-- 查看下一个问题 -->

@shaohuzhang1 shaohuzhang1 Nov 9, 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's my review of the code changes:

Issues & Suggestions

1. Significant flaw: knowledgeId.value usage in a plain function

return Boolean(perm.knowledge.workspace[method](knowledgeId.value))
  • knowledgeId is referenced but not shown as defined in this diff. If it's a ref declared after this function (e.g., below in <script setup>), this works at call time due to hoisting/closures — but it's fragile and confusing.
  • If knowledgeId is not a ref (e.g., a prop or computed differently), .value will be undefined. Verify its type.
  • Suggestion: Move the function below the knowledgeId declaration, or pass knowledgeId as an explicit parameter for clarity.

2. Style: missing spaces after commas

import { isWorkspaceSharedResource,isSystemResource, isSystemSharedResource } from '@/utils/resource-context'

Inconsistent spacing (isWorkspaceSharedResource,isSystemResource). Run Prettier/ESLint.

3. Missing return fallback / exhaustive check

KnowledgeProblemPermissionMethod currently has only one method ('problemRelate'), so the fallthrough return for workspace is fine today. But:

  • If more methods are added, be careful the perm.knowledge.* objects each expose the same keys.
  • Consider a default guard or assertNever(method) pattern if this union grows.

4. v-if placement on <el-button>

Template:

<el-button plain :disabled="loading" v-if="knowledgeProblemPermission('problemRelate')" @click="emit('associate')">关联分段</el-button>

This is functionally OK, but:

  • Attribute order: Conventionally put v-if first for readability: v-if="..." plain :disabled=....
  • Performance/correctness caveat: knowledgeProblemPermission('problemRelate') is called on every render. Since it reads reactive refs (knowledgeId.value) and permission state, Vue will re-evaluate reliably, but if perm.knowledge.* calls have side effects or are expensive, consider wrapping in a computed: 
    const canRelateProblem = computed(() => knowledgeProblemPermission('problemRelate'))
    Then use v-if="canRelateProblem".

5. isWorkspaceSharedResource() → false hardcoded

if (isWorkspaceSharedResource()) return false // workspace shared KB is read-only

Fine, but this bypasses any per-method logic. If later some methods should be allowed on workspace-shared resources, this becomes wrong. Document or extract into a map if it grows.

6. Naming: "问题" (problem) vs. variable problem

No functional issue, but knowledgeProblemPermission naming is consistent with the model name — good.

7. Redundant Boolean(...) casts

The Boolean() wrappers assume the perm.* methods may return non-boolean. That's defensive and OK, but if their signatures already guarantee boolean, the casts are noise. Keep only if you're unsure of the contract.

8. Order of checks

Current priority: system → system-shared → workspace-shared → workspace. Confirm that's intended precedence. A resource being both system and shared could match multiple predicates; make sure the priority matches business rules.

9. Import ordering

Imports are appended at the top-of-file region rather than sorted alongside existing @/ imports. Consistency with project import-order lint rule would fail CI if enforced.

Summary of recommended fixes

Priority Fix
High Confirm knowledgeId is a ref declared before use, or pass explicitly
Medium Wrap permission check in computed for the template
Medium Move v-if to be the first attribute
Low Fix comma spacing, import ordering
Low Add exhaustiveness handling if union grows

Otherwise the logic is sound and the feature (hiding "关联分段" when unauthorized) is implemented correctly.

Expand Down
2 changes: 1 addition & 1 deletion ui/src/views/knowledge-detail/problem/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ onMounted(() => loadProblems())
</template>
</MkTable>
<ProblemDetailDrawer
v-if="detailProblem && knowledgeProblemPermission('problemRelate')"
v-if="detailProblem && knowledgeProblemPermission('problemEdit')"
ref="problemDetailDrawerRef"
v-model="detailProblem"
:has-previous="hasPrevious"

@shaohuzhang1 shaohuzhang1 Nov 9, 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's my review of this diff:

Change summary: The permission check for rendering ProblemDetailDrawer was switched from 'problemRelate' to 'problemEdit'.

Potential issues / irregularities:

  1. Behavioral change needs verification — This is a semantic change, not a cosmetic one. Users who had problemRelate but not problemEdit will lose access to the drawer. Confirm this is intentional and matches the intended UX (e.g., whether "relating" problems legitimately requires edit rights).

  2. Permission granularity mismatch — Using problemEdit to control a "detail/relate" drawer couples read/relate operations to write permissions. If the drawer is primarily for viewing details, v-if on an edit-only permission may hide information users are otherwise allowed to see. Consider separating:

    • Viewing the drawer (problemRelate or a problemView)
    • Editing within it (problemEdit, checked inside the component or on action buttons)

  3. Naming/semantics of knowledgeProblemPermission — Make sure this helper returns a boolean consistently (not undefined/truthy objects), since it's used directly in v-if.

  4. Security note — Frontend v-if gating is UI-only; ensure the backend still enforces the same permission. A permission rename on the client without matching server checks can create inconsistent behavior.

  5. Regression risk with the item above/below references — If hasPrevious, pagination, or navigation logic assumed the drawer would render under problemRelate, verify that flow still works when the user lacks problemEdit. Consider guarding related refs/methods (e.g., problemDetailDrawerRef) against being null.

  6. No test update shown — Add/adjust a unit test asserting the drawer renders only with problemEdit (and does not for problemRelate-only users) to lock in the intent.

  7. Minor optimization — Not applicable here; the expression is already short-circuited (detailProblem && ...). No performance concern.

Recommendation: If the goal is "only editors can open the detail drawer," the change is fine — just add tests and confirm backend parity. If the drawer serves viewers too, prefer keeping problemRelate for visibility and gate only mutating actions behind problemEdit.

Expand Down
Loading