Skip to content

fix: application trigger create scope #7739

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: application trigger create scope #7739

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
2 changes: 1 addition & 1 deletion apps/common/auth/constants/permission_constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1718,7 +1718,7 @@ class PermissionConstants(Enum):
PermissionMeta(
role_list=[RoleConstants.WORKSPACE_MANAGE, RoleConstants.USER],
category=Category.WORKSPACE,
scope=[PermissionScopeConstants.WORKSPACE],
scope=[PermissionScopeConstants.WORKSPACE_RESOURCE, PermissionScopeConstants.WORKSPACE],
),
)

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

Change reviewed:

- scope=[PermissionScopeConstants.WORKSPACE],
+ scope=[PermissionScopeConstants.WORKSPACE_RESOURCE, PermissionScopeConstants.WORKSPACE],

Potential issues / irregularities

  1. Ordering inconsistency
    The scopes are now listed as WORKSPACE_RESOURCE first, then WORKSPACE. If scope lists elsewhere in the codebase are ordered from broadest to narrowest (or vice versa), this may break consistency. Check whether order matters when scopes are validated or matched (e.g., first-match logic).

  2. Semantics of combined scopes
    Combining WORKSPACE_RESOURCE and WORKSPACE means this permission can be granted both at workspace level and at individual resource level. Confirm that:

    • Downstream authorization logic correctly handles a permission that can be scoped both ways.
    • There isn't an implicit assumption elsewhere that a given permission maps to exactly one scope type.

  3. Duplicate/overlap risk
    If WORKSPACE_RESOURCE is conceptually a sub-scope of WORKSPACE, having both may be redundant or may cause double-counting in scope-expansion logic. Verify whether inheritance is already handled automatically elsewhere.

  4. Missing tests / no accompanying change
    This is a pure metadata change with behavioral implications for authz. Ensure tests cover:

    • A user with only WORKSPACE_RESOURCE scope.
    • A user with only WORKSPACE scope.
    • Checks for overlaps/ambiguity.

  5. Enum availability
    Confirm PermissionScopeConstants.WORKSPACE_RESOURCE actually exists and is defined consistently (name and value) across environments; a typo here would fail at import time rather than test time.

  6. Migration/compat concerns
    Existing role assignments created before this change won't have WORKSPACE_RESOURCE in their scope. If any code assumes the permission requires both scopes, existing users could suddenly be denied. Consider graceful fallback.

  7. Formatting/lint
    Line length may exceed the project's max (80/88/100 chars). Run the formatter/linter to confirm.

Optimization suggestions

  • Keep scope lists sorted deterministically (e.g., by enum definition order) to avoid noisy diffs and make ordering assumptions explicit.
  • If many permissions will gain WORKSPACE_RESOURCE, consider a helper/factory to reduce repetition and enforce consistent ordering.
  • Add an inline comment explaining why both scopes are allowed, since it's non-obvious from the diff alone.

Verdict

The change itself is syntactically fine, but it has real authorization semantics implications. The main risks are (a) ordering/duplicates, (b) behavior for pre-existing assignments without the new scope, and (c) lack of accompanying tests. Address these before merging.

Expand Down
Loading