feat(BA-2478): Add RBAC Global scope #6004

fregataa · 2025-09-22T13:10:18Z

Checklist: (if applicable)

Milestone metadata specifying the target backport version
Mention to the original issue
Installer updates including:
- Fixtures for db schema changes
- New mandatory config options
Update of end-to-end CLI integration tests in ai.backend.test
API server-client counterparts (e.g., manager API -> client SDK)
Test case(s) to:
- Demonstrate the difference of before/after
- Demonstrate the flow of abstract/conceptual models with a concrete implementation
Documentation
- Contents in the docs directory
- docstrings in public interfaces and type annotations

Copilot

Pull Request Overview

Adds a new GLOBAL RBAC scope and introduces system-level superadmin and monitor roles, along with a migration to create and map these roles to existing users. Key changes include extending enums with a GLOBAL scope, adding role creation helpers, and an Alembic migration to seed global-scope roles and permissions.

Added GLOBAL scope and related constants to RBAC enums and permission types
Introduced superadmin and monitor role creation helpers
New migration to create global roles, permission groups, and map users by legacy role

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.

File	Description
models/rbac_models/migration/user.py	Adds constants and helper functions for superadmin and monitor role creation
models/rbac_models/migration/enums.py	Extends OperationType and ScopeType with monitor/global concepts and introduces GLOBAL_SCOPE_ID
models/alembic/versions/09206ac04fd3_create_global_scope_roles.py	Migration to create global-scope roles, permissions, and map users
data/permission/types.py	Adds GLOBAL scope and duplicates GLOBAL_SCOPE_ID constant

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-22T13:14:50Z

src/ai/backend/manager/models/rbac_models/migration/user.py

    OperationType.GRANT_READ,
    OperationType.GRANT_UPDATE,
 }
+SUPERADMIN_OPERATIONS = OPERATIONS_IN_ROLE


[nitpick] SUPERADMIN_OPERATIONS references the same set object as OPERATIONS_IN_ROLE; if either is mutated later, both change unintentionally. Use a shallow copy to decouple: SUPERADMIN_OPERATIONS = OPERATIONS_IN_ROLE.copy().

Suggested change

SUPERADMIN_OPERATIONS = OPERATIONS_IN_ROLE

SUPERADMIN_OPERATIONS = OPERATIONS_IN_ROLE.copy()

Copilot · 2025-09-22T13:14:50Z

src/ai/backend/manager/models/rbac_models/migration/user.py

+def get_superadmin_role_creation_input() -> RoleCreateInput:
+    """
+    Create a superadmin role and permissions.
+    This role allows full access to all entities.
+    """
+    role_input = RoleCreateInput(
+        name=f"{ROLE_NAME_PREFIX}superadmin",
+        source=RoleSource.SYSTEM,


Docstring states this creates the role and permissions, but the function only returns a RoleCreateInput without creating permissions. Either adjust the wording (e.g., "Return RoleCreateInput for superadmin") or implement permission creation here for consistency.

Copilot · 2025-09-22T13:14:50Z

src/ai/backend/manager/models/rbac_models/migration/user.py

+def get_monitor_role_creation_input() -> RoleCreateInput:
+    """
+    Create a monitor role and permissions.
+    This role allows read-only access to all entities.
+    """
+    role_input = RoleCreateInput(
+        name=f"{ROLE_NAME_PREFIX}monitor",


Same mismatch as superadmin helper: the function returns only a RoleCreateInput and does not create permissions. Update docstring or extend functionality for clarity and consistency.

Copilot · 2025-09-22T13:14:51Z

src/ai/backend/manager/models/rbac_models/migration/enums.py

        return OriginalScopeType(self.value)


+GLOBAL_SCOPE_ID = ""


[nitpick] GLOBAL_SCOPE_ID is an empty string with no explanatory comment; using a sentinel empty string can be error-prone (e.g., falsy checks). Provide a comment explaining why an empty string is valid and consider using a clearly descriptive constant (e.g., "GLOBAL") to reduce ambiguity.

Suggested change

GLOBAL_SCOPE_ID = ""

# Sentinel value for the global scope. Use a descriptive string to avoid ambiguity and errors with falsy checks.

GLOBAL_SCOPE_ID = "GLOBAL"

Copilot · 2025-09-22T13:14:51Z

src/ai/backend/manager/data/permission/types.py

+    GLOBAL = "global"  # Global scope
+
+
+GLOBAL_SCOPE_ID = ""


[nitpick] GLOBAL_SCOPE_ID duplicates the same empty-string constant defined elsewhere (enums module). Centralize this definition to avoid divergence and document why an empty string is used.

Suggested change

GLOBAL_SCOPE_ID = ""

from ai.backend.manager.data.enums import GLOBAL_SCOPE_ID # Centralized definition; see enums module for documentation.

Copilot · 2025-09-22T13:14:52Z

src/ai/backend/manager/models/alembic/versions/09206ac04fd3_create_global_scope_roles.py

+        input = (
+            PermissionGroupCreateInput(
+                role_id=role_id,
+                scope_type=ScopeType.GLOBAL,
+                scope_id=GLOBAL_SCOPE_ID,
+            )
+        ).to_dict()


Variable name input shadows the built-in function input, which can confuse readers and tools. Rename to permission_group_data or pg_input for clarity.

Copilot · 2025-09-22T13:14:52Z

src/ai/backend/manager/models/alembic/versions/09206ac04fd3_create_global_scope_roles.py

+                input = PermissionCreateInput(
+                    permission_group_id=permission_group_id,
+                    entity_type=entity_type,
+                    operation=operation,
+                )
+                permission_inputs.append(input.to_dict())


Same shadowing of built-in input here; rename variable (e.g., perm_input) to avoid overshadowing and improve readability.

Copilot · 2025-09-22T13:14:52Z

src/ai/backend/manager/models/alembic/versions/09206ac04fd3_create_global_scope_roles.py

+class UserRole(enum.StrEnum):
+    """
+    User's role.
+    """
+
+    SUPERADMIN = "superadmin"
+    ADMIN = "admin"
+    USER = "user"
+    MONITOR = "monitor"


[nitpick] Redefining UserRole inside the migration duplicates existing role definitions elsewhere, increasing risk of drift if roles change. Consider importing the canonical enum or inlining plain string literals to avoid maintaining parallel enums in migrations.

Copilot · 2025-09-22T13:14:53Z

src/ai/backend/manager/models/alembic/versions/09206ac04fd3_create_global_scope_roles.py

+
+
+def downgrade() -> None:
+    pass


Missing downgrade logic means this migration is irreversible, which can hinder debugging or rollback scenarios. Implement removal of inserted roles, permission groups, permissions, and user-role mappings (or explicitly document why downgrade is intentionally unsupported).

Suggested change

pass

conn = op.get_bind()

roles_table = get_roles_table()

user_roles_table = get_user_roles_table()

permissions_table = get_permissions_table()

permission_groups_table = get_permission_groups_table()

# List of role names to remove

role_names = [UserRole.SUPERADMIN, UserRole.MONITOR]

# Find role IDs for these roles

role_rows = query_role_rows_by_name(conn, role_names)

role_ids = [row.id for row in role_rows]

if role_ids:

# Remove user-role mappings

conn.execute(

user_roles_table.delete().where(

user_roles_table.c.role_id.in_(role_ids)

)

)

# Remove permissions associated with these roles

conn.execute(

permissions_table.delete().where(

permissions_table.c.role_id.in_(role_ids)

)

)

# Remove permission groups associated with these roles

conn.execute(

permission_groups_table.delete().where(

permission_groups_table.c.role_id.in_(role_ids)

)

)

# Remove the roles themselves

conn.execute(

roles_table.delete().where(

roles_table.c.id.in_(role_ids)

)

)

Copilot · 2025-09-22T13:14:53Z

src/ai/backend/manager/models/rbac_models/migration/enums.py

+        """
+        return {
+            cls.READ,
+        }


[nitpick] monitor_operations duplicates member_operations logic (both return only READ). If the semantic distinction is intentional, add a clarifying comment; otherwise consider reusing member_operations to avoid duplication.

Suggested change

"""

return {

cls.READ,

}

This method reuses member_operations for consistency and to avoid duplication.

"""

return cls.member_operations()

HyeockJinKim · 2025-09-24T01:55:29Z

src/ai/backend/manager/data/permission/id.py

+    def is_global(self) -> bool:
+        return self.scope_type == ScopeType.GLOBAL


I’m not sure if this is really meaningful.

HyeockJinKim · 2025-09-24T02:11:20Z

src/ai/backend/manager/data/permission/role.py

 @dataclass
-class PermissionCheckInput:
+class MergedPermissionData:
+    scope_permissions: Mapping[ScopeId, set[OperationType]]
+    global_permissions: set[OperationType]
+    object_permissions: Mapping[ObjectId, set[OperationType]]


It seems like it would be good to separate it. However, I think we need to consider how to separate it a little bit, so I'll take a look and suggest some ideas.

HyeockJinKim · 2025-10-20T14:25:57Z

src/ai/backend/manager/data/permission/id.py

    scope_id: str

+    def __hash__(self) -> int:
+        return hash((self.scope_type, self.scope_id))


How about setting the frozen attribute of the dataclass?

It seems better to make values immutable rather than making mutable values hashable.

HyeockJinKim · 2025-10-20T14:38:35Z

src/ai/backend/manager/data/permission/role.py

+@dataclass
+class BatchEntityPermissionCheckInput:
+    user_id: uuid.UUID
+    target_object_ids: list[ObjectId]
+    operation: OperationType
+
+
+@dataclass
+class ScopePermissionSet:
+    scope_id: ScopeId
+    scope_permissions: set[OperationType]
+    global_permissions: Optional[set[OperationType]]
+
+
+@dataclass
+class ObjectPermissionSet:
+    object_id: ObjectId
+    object_permissions: set[OperationType]
+    mapped_scopes: dict[ScopeId, set[OperationType]]
+    global_permissions: Optional[set[OperationType]]


I think it would be good to keep most of the values frozen.

HyeockJinKim · 2025-10-20T15:22:30Z

src/ai/backend/manager/repositories/permission_controller/db_source.py

+                    sa.or_(
+                        PermissionGroupRow.scope_type == ScopeType.GLOBAL,
+                        PermissionGroupRow.scope_id == scope_id.scope_id,
+                    ),


Do we need to check the scope type?

I think we should also receive the value for the operation.

HyeockJinKim · 2025-10-20T15:30:00Z

src/ai/backend/manager/repositories/permission_controller/db_source.py

+            scope_permissions: set[OperationType] = set()
+            global_permissions: Optional[set[OperationType]] = None
+            for role in role_rows:
+                for pg in role.permission_group_rows:
+                    if pg.parsed_scope_id() != scope_id:
+                        continue
+                    if pg.scope_type == ScopeType.GLOBAL:
+                        global_permissions = {perm.operation for perm in pg.permission_rows}
+                    else:
+                        scope_permissions |= {perm.operation for perm in pg.permission_rows}
+
+        return ScopePermissionSet(scope_id, scope_permissions, global_permissions)


There doesn't seem to be a need to separate global permission specifically. What's actually needed is whether or not there's permission for the operation.

HyeockJinKim · 2025-10-20T15:31:06Z

src/ai/backend/manager/repositories/permission_controller/db_source.py

+    async def check_scope_permission_exist(
+        self,
+        user_id: uuid.UUID,
+        scope_id: ScopeId,
+        operation: OperationType,
+    ) -> bool:


If we are returning the permission set anyway, it doesn't seem necessary to have a separate check function.

src/ai/backend/manager/repositories/permission_controller/db_source.py

HyeockJinKim · 2025-10-20T15:39:27Z

src/ai/backend/manager/repositories/permission_controller/repository.py

    @permission_controller_repository_resilience.apply()
    async def check_permission_in_scope(self, data: ScopePermissionCheckInput) -> bool:
-        target_scope_id = data.target_scope_id
-        role_rows = await self._db_source.get_user_roles(data.user_id)
-        for role in role_rows:
-            for permission_group in role.permission_group_rows:
-                if permission_group.parsed_scope_id() != target_scope_id:
-                    continue
-                for permission in permission_group.permission_rows:
-                    if permission.operation == data.operation:
-                        return True
+        return await self._db_source.check_scope_permission_exist(
+            data.user_id, data.target_scope_id, data.operation
+        )


Due to the situation where a considerable amount of computation is performed in the DB, a cache layer is essential. As a follow-up task, please also apply a cache source.

HyeockJinKim · 2025-10-21T06:29:18Z

src/ai/backend/manager/data/permission/types.py

+
+    GLOBAL = "global"  # Global scope


Please remove line breaks.

HyeockJinKim · 2025-10-21T06:30:20Z

src/ai/backend/manager/repositories/permission_controller/db_source.py

+    async def get_object_permissions(
+        self,
+        user_id: uuid.UUID,
+        object_id: ObjectId,
+    ) -> ObjectPermissionSet:


You missed resilience decorator.

This is a db source module, not a repository module

fregataa added this to the 25Q2 milestone Sep 22, 2025

fregataa requested a review from HyeockJinKim September 22, 2025 13:10

fregataa self-assigned this Sep 22, 2025

Copilot AI review requested due to automatic review settings September 22, 2025 13:10

github-actions bot added size:L 100~500 LoC comp:manager Related to Manager component require:db-migration Automatically set when alembic migrations are added or updated labels Sep 22, 2025

Copilot AI reviewed Sep 22, 2025

View reviewed changes

fregataa marked this pull request as draft September 22, 2025 13:21

fregataa force-pushed the feat/add-rbac-global-scope branch 2 times, most recently from 7e5435c to c5407ac Compare September 24, 2025 01:43

HyeockJinKim reviewed Sep 24, 2025

View reviewed changes

fregataa force-pushed the feat/add-rbac-global-scope branch from c5407ac to d200694 Compare September 26, 2025 13:16

fregataa requested a review from HyeockJinKim September 26, 2025 13:16

fregataa marked this pull request as ready for review September 26, 2025 13:16

fregataa force-pushed the feat/add-rbac-global-scope branch from d0e4b56 to 88a06b5 Compare September 29, 2025 04:42

HyeockJinKim modified the milestones: 25Q2, 25.16 Oct 4, 2025

fregataa force-pushed the feat/add-rbac-global-scope branch from 88a06b5 to bd85f26 Compare October 13, 2025 06:50

fregataa force-pushed the feat/add-rbac-global-scope branch 2 times, most recently from 7c18773 to 46daf25 Compare October 20, 2025 12:25

HyeockJinKim reviewed Oct 20, 2025

View reviewed changes

feat: Add RBAC Global scope

6595a4b

HyeockJinKim reviewed Oct 21, 2025

View reviewed changes

remove method not used

e5d55cd

fregataa force-pushed the feat/add-rbac-global-scope branch from 46daf25 to e5d55cd Compare October 21, 2025 09:07

fregataa requested a review from HyeockJinKim October 21, 2025 09:27

HyeockJinKim approved these changes Oct 21, 2025

View reviewed changes

feat(BA-2477): Add superadmin and monitor roles (#6006)

3d69369

github-actions bot added size:XL 500~ LoC and removed size:L 100~500 LoC labels Oct 21, 2025

HyeockJinKim enabled auto-merge October 21, 2025 09:46

HyeockJinKim added this pull request to the merge queue Oct 21, 2025

auto-merge was automatically disabled October 21, 2025 09:56
Pull Request is not mergeable

Merged via the queue into main with commit ca26b1f Oct 21, 2025
31 checks passed

HyeockJinKim deleted the feat/add-rbac-global-scope branch October 21, 2025 09:58

	SUPERADMIN_OPERATIONS = OPERATIONS_IN_ROLE
	SUPERADMIN_OPERATIONS = OPERATIONS_IN_ROLE.copy()

	GLOBAL_SCOPE_ID = ""
	# Sentinel value for the global scope. Use a descriptive string to avoid ambiguity and errors with falsy checks.
	GLOBAL_SCOPE_ID = "GLOBAL"

	GLOBAL_SCOPE_ID = ""
	from ai.backend.manager.data.enums import GLOBAL_SCOPE_ID # Centralized definition; see enums module for documentation.

-    pass
+    conn = op.get_bind()
+    roles_table = get_roles_table()
+    user_roles_table = get_user_roles_table()
+    permissions_table = get_permissions_table()
+    permission_groups_table = get_permission_groups_table()
+    # List of role names to remove
+    role_names = [UserRole.SUPERADMIN, UserRole.MONITOR]
+    # Find role IDs for these roles
+    role_rows = query_role_rows_by_name(conn, role_names)
+    role_ids = [row.id for row in role_rows]
+    if role_ids:
+        # Remove user-role mappings
+        conn.execute(
+            user_roles_table.delete().where(
+                user_roles_table.c.role_id.in_(role_ids)
+            )
+        )
+        # Remove permissions associated with these roles
+        conn.execute(
+            permissions_table.delete().where(
+                permissions_table.c.role_id.in_(role_ids)
+            )
+        )
+        # Remove permission groups associated with these roles
+        conn.execute(
+            permission_groups_table.delete().where(
+                permission_groups_table.c.role_id.in_(role_ids)
+            )
+        )
+        # Remove the roles themselves
+        conn.execute(
+            roles_table.delete().where(
+                roles_table.c.id.in_(role_ids)
+            )
+        )

		def is_global(self) -> bool:
		return self.scope_type == ScopeType.GLOBAL

feat(BA-2478): Add RBAC Global scope #6004

feat(BA-2478): Add RBAC Global scope #6004

Uh oh!

Conversation

fregataa commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fregataa commented Sep 22, 2025 •

edited

Loading