Skip to content

fix: enforce detailed permissions for resource authorization #7735

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: enforce detailed permissions for resource authorization #7735

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
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 @@ -269,7 +269,7 @@ class PermissionConstants(Enum):
PermissionMeta(
role_list=[RoleConstants.WORKSPACE_MANAGE, RoleConstants.USER],
category=Category.WORKSPACE,
scope=[PermissionScopeConstants.WORKSPACE_RESOURCE],
scope=[PermissionScopeConstants.WORKSPACE, PermissionScopeConstants.WORKSPACE_RESOURCE],
resource_permission_group_list=[ResourcePermissionConst.MODEL_MANAGE],
),
)

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

Review of the change

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

Correctness / potential issues

  1. Privilege escalation risk (most important). Adding WORKSPACE broadens where this permission is evaluated. If the authorization layer grants access when any scope in the list matches, this permission now applies at the whole-workspace level in addition to the resource level. Confirm this is intentional and that the role list (WORKSPACE_MANAGE, USER) should really get the workspace-wide variant. USER in particular may now pass checks it previously failed.

  2. Scope precedence / evaluation order. If scope resolution is order-sensitive (first match wins, or a single "effective scope" is derived), the new first element changes behavior beyond just "adding" a scope. Verify the consumer iterates all scopes (any(...)) rather than taking scope[0].

  3. Deduplication / caching. If the scope list is used as a cache key, serialized into tokens, or persisted in a DB, adding an element invalidates existing entries and may require a data migration or cache bust.

  4. Consistency with sibling permission entries. Check whether other PermissionMeta entries under Category.WORKSPACE with ResourcePermissionConst.MODEL_MANAGE already include WORKSPACE. If not, this one becomes inconsistent and may indicate the change belongs in a different place (or that several entries need the same adjustment).

  5. Tests and snapshots. Any test asserting the exact scope list, role–permission matrices, or serialized permission payloads will need updating. Add a positive test for workspace-level access and a negative test confirming roles that shouldn't gain access still can't.

  6. Trailing comma style. The original had a trailing comma in a single-element list; the new version drops it. Harmless, but keep formatting consistent with the project's linter (e.g., Black would re-add it across two lines).

Minor / optimization suggestions

  • Use an immutable container. If scope is never mutated, prefer frozenset/tuple over list to prevent accidental in-place changes and to make the value hashable for caching.
  • Format multi-line for readability if the line exceeds the project's max length: 
    scope=[
    PermissionScopeConstants.WORKSPACE,
    PermissionScopeConstants.WORKSPACE_RESOURCE,
],
  • Add a comment explaining why both scopes are required, since the intent isn't obvious from the code.
  • Guard against duplicates if this list is ever built dynamically (e.g., ordered_unique(...)), so a duplicated scope can't skew precedence logic.

Bottom line

The syntax is fine; the change is a semantic broadening of an authorization scope and should be treated as a security-relevant change. Verify the scope evaluation logic, confirm only the intended roles benefit, and update tests/data accordingly.

Expand Down
Loading
Loading