Skip to content

fix: correct role reference for EXTENDS_ADMIN in workspace permissions #7726

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

fix: correct role reference for EXTENDS_ADMIN in workspace permissions #7726

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/users/views/user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ class WorkspaceUserMemberView(APIView):
@has_permissions(
RoleConstants.WORKSPACE_MANAGE.get_workspace_role(),
RoleConstants.ADMIN,
RoleConstants.EXTENDS_ADMIN.get_workspace_role(),
RoleConstants.EXTENDS_ADMIN,
RoleConstants.EXTENDS_WORKSPACE_MANAGE.get_workspace_role(),
RoleConstants.USER.get_workspace_role(),
RoleConstants.EXTENDS_USER.get_workspace_role(),

@shaohuzhang1 shaohuzhang1 Nov 6, 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.

The diff is a small permission-decorator change:

- RoleConstants.EXTENDS_ADMIN.get_workspace_role(),
+ RoleConstants.EXTENDS_ADMIN,

Assessment

  • No syntax issue is visible in the diff.
  • Correctness cannot be fully confirmed without seeing RoleConstants and has_permissions(...).
  • The main risk is whether has_permissions expects a workspace-scoped role or a global/base role.

Potential issues

  1. Inconsistent role normalization

    • Most entries call .get_workspace_role(), but RoleConstants.ADMIN and now RoleConstants.EXTENDS_ADMIN do not.
    • If EXTENDS_ADMIN has a get_workspace_role() method, this change may accidentally pass the wrong role representation and cause:
      • permission denial, or
      • worse, unintended permission grant if the decorator compares loosely.
    • If EXTENDS_ADMIN is already a workspace-level/global role and has no such method, the change is likely correct.

  2. Possible security impact

    • This is authorization code. A wrong role constant can silently grant or revoke access.
    • Verify that the raw RoleConstants.EXTENDS_ADMIN value matches the value stored/expected by has_permissions.

  3. Consistency with ADMIN

    • RoleConstants.ADMIN is already passed raw. If that is intentional because admins are global, then EXTENDS_ADMIN being raw may also be intentional.
    • If not, both should likely be normalized the same way as the other roles.

Recommendations

  • Confirm the contract of has_permissions: does it accept global roles, workspace roles, or both?
  • Check whether RoleConstants.EXTENDS_ADMIN has get_workspace_role():
    • If yes, prefer calling it for consistency unless there is a documented reason not to.
    • If no, the change is likely a bug fix.
  • Add a short comment explaining why ADMIN and EXTENDS_ADMIN are passed differently, if intentional.

Suggested refactor

Centralize the allowed roles to avoid forgetting or misusing .get_workspace_role():

WORKSPACE_USER_MEMBER_ROLES = (
    RoleConstants.WORKSPACE_MANAGE.get_workspace_role(),
    RoleConstants.ADMIN,
    RoleConstants.EXTENDS_ADMIN,
    RoleConstants.EXTENDS_WORKSPACE_MANAGE.get_workspace_role(),
    RoleConstants.USER.get_workspace_role(),
    RoleConstants.EXTENDS_USER.get_workspace_role(),
)

Then:

@has_permissions(*WORKSPACE_USER_MEMBER_ROLES)

This improves readability, consistency, and testability.

Tests to add

Add permission-matrix tests covering:

  • ADMIN
  • EXTENDS_ADMIN
  • WORKSPACE_MANAGE
  • EXTENDS_WORKSPACE_MANAGE
  • USER
  • EXTENDS_USER

Verify that each role gets the expected allow/deny result, especially around EXTENDS_ADMIN.

Verdict: The change may be correct if EXTENDS_ADMIN is already the value expected by has_permissions. Otherwise, it introduces a permission-matching bug. Verify the RoleConstants definition and add a regression test for EXTENDS_ADMIN.

Expand Down
Loading