Skip to content

fix: workspace trigger permission #7921

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

fix: workspace trigger permission #7921

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
11 changes: 9 additions & 2 deletions ui/src/permission/modules/trigger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,18 @@
/** 工作空间触发器操作权限。 */
import { can } from '../policy'
import { PermissionConstants as P } from '../core'
import { hasPermission, PermissionConstants as P } from '../core'
import { AggregatePermission, Role, RoleConstants } from '../core/common'
import { getWorkspaceId } from '@/utils/resource-context'
export default {
/**
* 工作空间「触发器」只读权限
*/
read: () => can(P.TRIGGER_READ),
read: () => {
const workspaceId=getWorkspaceId() || ''
return hasPermission(AggregatePermission.builder()
.addRole(new Role(RoleConstants.WORKSPACE_MANAGE.name, workspaceId))
.addPermission(P.TRIGGER_READ.getWorkspacePermission()({'workspaceId':workspaceId})).build())
},
/**
* 工作空间「触发器」创建权限
*/

@shaohuzhang1 shaohuzhang1 Nov 13, 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, highlighting irregularities, potential issues, and optimization suggestions.

1. Irregularities & Code Style Issues

  • Missing Spaces: There are missing spaces after keywords and operators, which violates standard JavaScript/TypeScript formatting (e.g., workspaceId=getWorkspaceId(), (new Role(...))).
  • Poor Indentation: The code inside the read function has inconsistent and insufficient indentation. The .addPermission line does not align visually with .addRole.
  • Long Lines: The return statement is extremely long and hard to read. It chains multiple method calls on one line.

2. Potential Logic Issues

  • Empty workspaceId Handling:

    const workspaceId = getWorkspaceId() || ''

    If getWorkspaceId() returns an empty string or undefined, it defaults to ''. Passing an empty string as a key in {'workspaceId': ''} might cause unexpected permission behavior or bugs in downstream logic that expects a valid ID. Consider explicitly handling the case where the workspace ID is missing (e.g., returning false or throwing an error if context is required).

  • Implicit Async Behavior: If hasPermission or can were asynchronous (though likely not here based on typical permission libraries), this code would fail. Assuming they are synchronous, it's fine, but verify the signature of hasPermission.

  • Redundant Permission Check?:
    You are adding:

    1. A Role-based permission: WORKSPACE_MANAGE role for the specific workspace.
    2. A Direct permission: P.TRIGGER_READ.getWorkspacePermission()....

    Ensure this is intentional. Usually, having the WORKSPACE_MANAGE role implies read access to triggers. If hasPermission uses an OR-logic (allow if any pass), ensure the direct permission isn't redundant. If it uses AND-logic, both conditions must be met. Clarify the semantics of AggregatePermission.builder().build() vs hasPermission() evaluation logic.

3. Optimization & Best Practice Suggestions

  • Use Modern JS Syntax: Prefer const and arrow functions consistently (already done, but spacing matters).
  • Break Down Complex Expressions: Split the builder pattern into multiple lines for readability.
  • Avoid Magic Strings/Keys: The object {'workspaceId':workspaceId} should use shorthand if possible or ensure it matches the expected API contract strictly.
  • Consistency with Other Methods: The comment shows other methods like create. Ensure consistency in how they fetch workspace context and check permissions (e.g., do they also use hasPermission now, or still can?). Mixed approaches lead to maintenance nightmares.

✅ Suggested Refactored Code

/** 工作空间触发器操作权限。 */
import { can } from '../policy'
import { hasPermission, PermissionConstants as P } from '../core'
import { AggregatePermission, Role, RoleConstants } from '../core/common'
import { getWorkspaceId } from '@/utils/resource-context'

export default {
  /**
   * 工作空间「触发器」只读权限
   */
  read: () => {
    const workspaceId = getWorkspaceId()
    
    // Optional: Handle missing workspace context explicitly
    if (!workspaceId) {
      return false 
    }

    const aggregatePerm = AggregatePermission.builder()
      .addRole(new Role(RoleConstants.WORKSPACE_MANAGE.name, workspaceId))
      .addPermission(
        P.TRIGGER_READ.getWorkspacePermission()({ workspaceId })
      )
      .build()

    return hasPermission(aggregatePerm)
  },

  /**
   * 工作空间「触发器」创建权限
   */
  // ... rest of the file
}

Key Improvements in Refactor:

  1. Readability: Broken down into multiple steps.
  2. Early Return: Handles missing workspaceId early to prevent passing empty strings.
  3. Shorthand Property: { workspaceId } instead of { workspaceId: workspaceId }.
  4. Formatting: Proper spacing and indentation applied.

Expand Down
Loading