Skip to content

refactor: update workspace manage role usage in permissions #7838

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

refactor: update workspace manage role usage in permissions #7838

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
32 changes: 32 additions & 0 deletions apps/users/tests/test_user_workspace_permission.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
from types import SimpleNamespace
from unittest.mock import patch

from django.test import SimpleTestCase

from common.auth.struct.auth import Auth
from common.exception.app_exception import AppUnauthorizedFailed
from users.views.user import WorkspaceUserListView


class WorkspaceUserListPermissionTests(SimpleTestCase):
def _request(self, roles):
return SimpleNamespace(
auth=Auth(roles=set(roles), permissions={}),
user=SimpleNamespace(id="user-1"),
query_params=SimpleNamespace(get=lambda *args, **kwargs: None),
)

def test_workspace_scoped_manage_role_can_access_user_list(self):
request = self._request(["WORKSPACE_MANAGE:w:workspace-1"])
with patch("users.views.user.UserManageSerializer.get_user_list", return_value=[]):
try:
result = WorkspaceUserListView().get(request, workspace_id="workspace-1")
except AppUnauthorizedFailed as exc:
self.fail(f"workspace user denied despite matching workspace role: {exc}")
self.assertEqual(result.status_code, 200)

def test_global_workspace_role_alone_cannot_access_workspace_user_list(self):
request = self._request(["WORKSPACE_MANAGE"])
with patch("users.views.user.UserManageSerializer.get_user_list", return_value=[]):
with self.assertRaises(AppUnauthorizedFailed):
WorkspaceUserListView().get(request, workspace_id="workspace-1")

@shaohuzhang1 shaohuzhang1 Nov 11, 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 are the issues and suggestions for this test code:

Potential Issues

  1. SimpleNamespace used for query_params — Only .get() is stubbed. If WorkspaceUserListView.get() (or a serializer it calls) iterates, uses __getitem__, keys(), or checks truthiness of query_params, these tests will raise AttributeError. Prefer a real django.http.QueryDict({}, mutable=False) or RequestFactory — it's free and matches production behavior.

  2. SimpleNamespace request instead of a real request object — Bypasses middleware, request.GET, and any attribute access order. If the view ever reads request.method, request.path, or DRF-style attributes, the test breaks for the wrong reason. Consider self.client.get(...) with an overridden auth, or RequestFactory + manual auth injection.

  3. Auth construction assumptions — Test assumes Auth(roles=set, permissions={}) is a valid constructor and that roles are plain strings like "WORKSPACE_MANAGE:w:workspace-1". This couples the test to an internal string format; if role encoding changes (e.g., tuple format), all these tests silently mis-test. Verify against the actual Auth implementation.

  4. Patching a specific call path (users.views.user.UserManageSerializer.get_user_list) — brittle: refactors (renaming class, moving method, renaming imported symbol) break the test without changing behavior. Also note: patching get_user_list on the class means the self argument binding depends on how it's called (class vs instance). Ensure you're not accidentally patching a static/instance-method mismatch — a MagicMock assigned as a class attribute will receive self as first arg unless bound correctly, which can mask bugs.

  5. Assertion on result.status_code == 200 without asserting response content — if the view returns an error payload with status 200 (common in some frameworks returning {"code": ...}), the test passes erroneously. Assert on body/empty list too.

  6. except ... self.fail(...) pattern — fine, but self.fail inside try after catching will re-raise AssertionError; ensure AppUnauthorizedFailed isn't a subclass of Exception hierarchy where self.fail triggers further exception handling. Prefer assertRaises / positive assertion without try/except, or use self.assertNotRaises helper.

Style / Minor

  1. Magical string literal "workspace-1" repeated — define a constant to avoid typo drift between request setup and assert.
  2. No test for mismatched workspace — e.g., role scoped to workspace-2 requesting workspace-1 should be denied. This is the key security boundary; currently untested.
  3. permissions={} empty — no test verifying that added permissions don't bypass scoping.
  4. SimpleTestCase may fail at runtime if DB access occurs (serializer could hit DB); use TestCase if there's any chance of DB queries.
  5. Missing async consideration — if the view supports async, tests should reflect the sync path being called intentionally.
  6. Patch target duplication — extract patch string into a module-level constant or use patch.object.

Suggested Improvements (summary)

  • Use RequestFactory/QueryDict instead of SimpleNamespace.
  • Add negative tests: mismatched workspace ID, missing role, role with wrong suffix.
  • Assert on response data, not just status.
  • Avoid patching deep internals; prefer testing via public API or injecting dependencies.
  • Parameterize the two tests with subTest to reduce duplication.

24 changes: 18 additions & 6 deletions apps/users/views/system_user_group.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class SystemUserGroupView(APIView):
PermissionConstants.SYSTEM_USER_GROUP_CREATE,
PermissionConstants.SYSTEM_USER_GROUP_EDIT,
RoleConstants.ADMIN,
RoleConstants.WORKSPACE_MANAGE,
RoleConstants.WORKSPACE_MANAGE.get_workspace_role(),
)
@log(
menu="IAM/System User Group",
Expand All @@ -87,7 +87,11 @@ def post(self, request: Request, workspace_id: str):
responses=UserGroupListApi.get_response(),
tags=["IAM/System User Group"],
)
@has_permissions(PermissionConstants.SYSTEM_USER_GROUP_READ, RoleConstants.ADMIN, RoleConstants.WORKSPACE_MANAGE)
@has_permissions(
PermissionConstants.SYSTEM_USER_GROUP_READ,
RoleConstants.ADMIN,
RoleConstants.WORKSPACE_MANAGE.get_workspace_role(),
)
def get(self, request: Request, workspace_id: str):
return result.success(SystemUserGroupCreateSerializer.Query(data={"workspace_id": workspace_id}).list())

Expand All @@ -104,7 +108,9 @@ class Delete(APIView):
tags=["IAM/System User Group"],
)
@has_permissions(
PermissionConstants.SYSTEM_USER_GROUP_DELETE, RoleConstants.ADMIN, RoleConstants.WORKSPACE_MANAGE
PermissionConstants.SYSTEM_USER_GROUP_DELETE,
RoleConstants.ADMIN,
RoleConstants.WORKSPACE_MANAGE.get_workspace_role(),
)
@log(
menu="IAM/System User Group",
Expand Down Expand Up @@ -132,7 +138,9 @@ class AddMember(APIView):
tags=["IAM/System User Group"],
)
@has_permissions(
PermissionConstants.SYSTEM_USER_GROUP_ADD_MEMBER, RoleConstants.ADMIN, RoleConstants.WORKSPACE_MANAGE
PermissionConstants.SYSTEM_USER_GROUP_ADD_MEMBER,
RoleConstants.ADMIN,
RoleConstants.WORKSPACE_MANAGE.get_workspace_role(),
)
@log(
menu="IAM/System User Group",
Expand Down Expand Up @@ -163,7 +171,9 @@ class RemoveMember(APIView):
tags=["IAM/System User Group"],
)
@has_permissions(
PermissionConstants.SYSTEM_USER_GROUP_REMOVE_MEMBER, RoleConstants.ADMIN, RoleConstants.WORKSPACE_MANAGE
PermissionConstants.SYSTEM_USER_GROUP_REMOVE_MEMBER,
RoleConstants.ADMIN,
RoleConstants.WORKSPACE_MANAGE.get_workspace_role(),
)
@log(
menu="IAM/System User Group",
Expand Down Expand Up @@ -193,7 +203,9 @@ class UserList(APIView):
responses=SystemUserGroupUserListPageResponse,
)
@has_permissions(
PermissionConstants.SYSTEM_USER_GROUP_READ, RoleConstants.ADMIN, RoleConstants.WORKSPACE_MANAGE
PermissionConstants.SYSTEM_USER_GROUP_READ,
RoleConstants.ADMIN,
RoleConstants.WORKSPACE_MANAGE.get_workspace_role(),
)
def get(self, request: Request, workspace_id: str, user_group_id: str, current_page: int, page_size: int):
d = UserGroupListPageSerializer.Query(

@shaohuzhang1 shaohuzhang1 Nov 11, 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 diff

Summary

The change replaces RoleConstants.WORKSPACE_MANAGE with RoleConstants.WORKSPACE_MANAGE.get_workspace_role() in all @has_permissions decorators. This looks intentional and consistent across every call site.

Observations / Potential Issues

  1. Method call at decoration time (evaluated once at import)

    • get_workspace_role() is now invoked at class/module import time, not per-request. This is fine only if the role value is static. If it depends on runtime state (workspace, user, settings), this is a bug. Worth confirming the method is pure/static.
    • If WORKSPACE_MANAGE had other attributes consumed elsewhere as an enum-like constant, converting via a method makes call sites less uniform. Consider exposing a separate constant (e.g., RoleConstants.WORKSPACE_ROLE) instead of a method call inside decorators.

  2. Consistency concern

    • Only these permission checks were changed. Grep the codebase for other usages of RoleConstants.WORKSPACE_MANAGE to ensure they don’t also need get_workspace_role(). Mismatched usage can lead to authorization bypass or denial.

  3. Import / side-effect ordering

    • Because decorators are evaluated at import, any circular import or initialization-order issue in RoleConstants.get_workspace_role() will now surface at import time rather than request time. Verify no lazy/DI dependency.

  4. Type / return value

    • Ensure get_workspace_role() returns the same type that has_permissions expects (string, enum, list?). A silent type change could make checks always false or always true.

  5. RoleConstants.ADMIN left unchanged

    • Confirm ADMIN does not have an equivalent scope-dependent form; otherwise this is inconsistent.

  6. Duplication

    • The triple (PermissionConstant.X, RoleConstants.ADMIN, RoleConstants.WORKSPACE_MANAGE.get_workspace_role()) is repeated many times. Consider extracting a helper, e.g.: 
      WORKSPACE_MANAGE_ROLES = (RoleConstants.ADMIN, RoleConstants.WORKSPACE_MANAGE.get_workspace_role())
      then use @has_permissions(PermissionConstants.SYSTEM_USER_GROUP_READ, *WORKSPACE_MANAGE_ROLES). This reduces drift.

  7. Testability

    • Add/adjust tests asserting the decorated endpoints still grant workspace-manage users access and deny others. A silent "always deny" from a wrong return type is easy to miss in code review.

  8. Line length / formatting

    • The multi-line reformat is good; no issues.

Minor nits

  • No docstring/comment explains why get_workspace_role() is required vs. the constant. A brief comment near the first usage would help future readers.
  • Consider making get_workspace_role() a @classmethod or module-level function if it has no instance state, to clarify evaluation semantics.

Overall

The patch is consistent within this file and appears safe provided get_workspace_role() is pure, statically returns the expected role type, and all other call sites using WORKSPACE_MANAGE are updated identically. Recommend grepping the repo and adding authorization tests.

Expand Down
8 changes: 4 additions & 4 deletions apps/users/views/user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,12 +180,12 @@ class WorkspaceUserListView(APIView):
responses=WorkspaceUserAPI.get_response(),
)
@has_permissions(
RoleConstants.WORKSPACE_MANAGE,
RoleConstants.WORKSPACE_MANAGE.get_workspace_role(),
RoleConstants.ADMIN,
RoleConstants.EXTENDS_ADMIN,
RoleConstants.EXTENDS_WORKSPACE_MANAGE,
RoleConstants.USER,
RoleConstants.EXTENDS_USER,
RoleConstants.EXTENDS_WORKSPACE_MANAGE.get_workspace_role(),
RoleConstants.USER.get_workspace_role(),
RoleConstants.EXTENDS_USER.get_workspace_role(),
)
def get(self, request: Request, workspace_id):
nick_name = request.query_params.get("nick_name", None)

@shaohuzhang1 shaohuzhang1 Nov 11, 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:

Potential Issues

1. Inconsistent transformation (likely a bug)

Two entries are left untransformed:

  • RoleConstants.ADMIN
  • RoleConstants.EXTENDS_ADMIN

But the similar "manage" and "user" roles got .get_workspace_role() applied. If the intent is that all workspace-scoped roles should be resolved via get_workspace_role(), then ADMIN/EXTENDS_ADMIN should probably be treated consistently (either both transformed or both left as-is). Verify whether ADMIN should also map to its workspace variant — if so, this patch is incomplete.

2. Assumption that get_workspace_role() exists on every constant

You're calling .get_workspace_role() on class attributes without any guard. If RoleConstants.WORKSPACE_MANAGE, EXTENDS_WORKSPACE_MANAGE, USER, or EXTENDS_USER are ever plain strings/enums without that method, this raises AttributeError at import/class-definition time. Confirm these constants are always objects exposing that method.

3. Decorator evaluated at class-load time

Because @has_permissions(...) runs at import, get_workspace_role() is called once per process start. If role definitions can change at runtime (e.g., DB-driven), the permission set will be stale. Probably fine for static config, but worth confirming it isn't intended to be dynamic.

4. Ordering / overlap semantics

After transformation some "extend" and base roles may collapse to the same value (e.g., if EXTENDS_USER.get_workspace_role() == USER.get_workspace_role()). Duplicate permissions passed to has_permissions are usually harmless but can mask logic errors. Worth asserting they remain distinct.

5. Behavioral change may widen/narrow access

If get_workspace_role() maps a global role to a workspace-scoped equivalent, this silently changes who can call GET. Ensure this matches the authorization design and add/extend tests covering each of the five role paths, plus an unauthorized case.

Optimization Suggestions

  • Centralize the mapping: instead of repeating .get_workspace_role() in every call site, define a helper (e.g., a module-level tuple WORKSPACE_USER_LIST_ROLES) or build the list once. This avoids drift like the inconsistent ADMIN handling above.
  • Deduplicate defensively: roles = list(dict.fromkeys(...)) or a set before passing, to guarantee no duplicate permissions.
  • Precompute the full role list at module scope rather than inside the decorator call, so it's obvious and testable.
  • Add a unit test that asserts the exact expected tuple of roles for this view, catching future discrepancies automatically.

Verdict

The change looks directionally correct, but the inconsistent handling of ADMIN/EXTENDS_ADMIN versus the other roles is the main thing to verify — it's either an intentional distinction that deserves a comment, or an oversight.

Expand Down
Loading