Skip to content

fix: ui permission #7734

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

fix: ui permission #7734

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
1 change: 1 addition & 0 deletions ui/src/permission/core/data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ export const PermissionConstants = {
APPLICATION_PUBLISH: p(Group.APPLICATION, Group.APPLICATION, Operate.PUBLISH, 7),
APPLICATION_BATCH_DELETE: p(Group.APPLICATION, Group.APPLICATION, Operate.BATCH_DELETE, 8),
APPLICATION_BATCH_MOVE: p(Group.APPLICATION, Group.APPLICATION, Operate.BATCH_MOVE, 9),
APPLICATION_CLEAR_POLICY: p(Group.APPLICATION, Group.APPLICATION, Operate.CLEAR_POLICY, 42),
APPLICATION_RESOURCE_AUTHORIZATION: p(Group.APPLICATION, Group.APPLICATION, Operate.AUTH, 10),
APPLICATION_RELATE_RESOURCE_VIEW: p(Group.APPLICATION, Group.APPLICATION, Operate.RELATE_VIEW, 11),
APPLICATION_TRIGGER_READ: p(Group.APPLICATION, Group.APPLICATION, Operate.TRIGGER_READ, 12),

@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 diff:

Irregularities / Potential Issues

  1. Magic number 42 for the permission code. Other entries use sequential codes (7, 8, 9, 10, 11, 12). The jump to 42 is suspicious—likely conflicts with, or leaves gaps from, other permissions. Verify:

    • That 42 isn't already assigned elsewhere in this file or sibling constant files.
    • Whether an existing constant/enum (e.g., Operate.CLEAR_POLICY) already defines the numeric value, so you can reference it instead of hardcoding.
    • That the intended behavior is "gap allowed" rather than a typo (e.g., should be 9.5-style or 13).

  2. Operate.CLEAR_POLICY may not exist yet. If this is the first usage, ensure it's added to the Operate enum and that any backend/permission-mapping tables are updated in the same change—otherwise runtime lookups will fail.

  3. Ordering inconsistency. The new entry is inserted between BATCH_MOVE (9) and RESOURCE_AUTHORIZATION (10), but its code is 42. This breaks the visual/numeric ordering convention of the block and makes future diffs harder to scan. Consider placing it at the end of the APPLICATION_* group or beside other high-numbered codes.

  4. Naming/scope mismatch (minor). The key is APPLICATION_CLEAR_POLICY but both groups are Group.APPLICATION. Confirm this permission is genuinely application-scoped and not resource-level; if it belongs elsewhere, the group args should reflect that.

  5. Backward compatibility. Adding a permission to a shared constants export can affect serialization (e.g., if consumers enumerate PermissionConstants and persist indices). Confirm no consumer relies on positional/contiguous numbering.

Optimization Suggestions

  • Define the code via a named constant (e.g., Operate.CLEAR_POLICY = 42 or a CODE map) rather than an inline literal, to keep a single source of truth.
  • Add a brief comment noting why 42 is used (if intentional) or renumber.
  • If permission codes must remain unique across modules, add/extend a unit test asserting uniqueness of all values in PermissionConstants.
  • Ensure corresponding authorization tests cover the new permission.

Summary: Functionally likely fine, but verify the 42 code doesn't collide, confirm Operate.CLEAR_POLICY is defined, and consider ordering/naming consistency plus a uniqueness test.

Expand Down
Loading