Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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 src/ai/backend/manager/actions/action/rbac_registry.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
"""Central RBAC action registry list.

This module provides the canonical list of all RBAC action classes
that should be registered in the PermissionControllerService.
Keeping this list in a lightweight module allows tests to verify
registry completeness without importing the full service factory.
"""

from ai.backend.manager.actions.action.rbac import BaseRBACAction
from ai.backend.manager.actions.action.rbac_session import (
SessionCreateRBACAction,
SessionGetRBACAction,
SessionGrantAllRBACAction,
SessionGrantHardDeleteRBACAction,
SessionGrantReadRBACAction,
SessionGrantUpdateRBACAction,
SessionHardDeleteRBACAction,
SessionSearchRBACAction,
SessionUpdateRBACAction,
)

RBAC_ACTION_REGISTRY: list[type[BaseRBACAction]] = [
SessionCreateRBACAction,
SessionGetRBACAction,
SessionSearchRBACAction,
SessionUpdateRBACAction,
SessionHardDeleteRBACAction,
SessionGrantAllRBACAction,
SessionGrantReadRBACAction,
SessionGrantUpdateRBACAction,
SessionGrantHardDeleteRBACAction,
]
Comment on lines +22 to +32
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RBAC_ACTION_REGISTRY is a module-level constant but is defined as a mutable list. Since factory.create_services() passes this object through directly, any accidental mutation would affect all consumers (and could create hard-to-debug global state). Consider making it immutable (e.g., Final + tuple[...]) and type it as a Sequence[type[BaseRBACAction]]/tuple[...] to communicate intent and prevent mutation.

Copilot uses AI. Check for mistakes.
24 changes: 2 additions & 22 deletions src/ai/backend/manager/services/factory.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,4 @@
from ai.backend.manager.actions.action.rbac_session import (
SessionCreateRBACAction,
SessionGetRBACAction,
SessionGrantAllRBACAction,
SessionGrantHardDeleteRBACAction,
SessionGrantReadRBACAction,
SessionGrantUpdateRBACAction,
SessionHardDeleteRBACAction,
SessionSearchRBACAction,
SessionUpdateRBACAction,
)
from ai.backend.manager.actions.action.rbac_registry import RBAC_ACTION_REGISTRY
from ai.backend.manager.actions.monitors.monitor import ActionMonitor
from ai.backend.manager.actions.validators import ActionValidators
from ai.backend.manager.services.agent.processors import AgentProcessors
Expand Down Expand Up @@ -290,17 +280,7 @@ def create_services(args: ServiceArgs) -> Services:
),
permission_controller=PermissionControllerService(
repository=repositories.permission_controller.repository,
rbac_action_registry=[
SessionCreateRBACAction,
SessionGetRBACAction,
SessionSearchRBACAction,
SessionUpdateRBACAction,
SessionHardDeleteRBACAction,
SessionGrantAllRBACAction,
SessionGrantReadRBACAction,
SessionGrantUpdateRBACAction,
SessionGrantHardDeleteRBACAction,
],
rbac_action_registry=RBAC_ACTION_REGISTRY,
),
vfs_storage=VFSStorageService(
vfs_storage_repository=repositories.vfs_storage.repository,
Expand Down
61 changes: 61 additions & 0 deletions tests/unit/manager/actions/test_rbac_registry.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
"""Tests for RBAC action registry completeness."""

import importlib
import inspect
import pkgutil

import ai.backend.manager.actions.action as action_pkg
from ai.backend.manager.actions.action.rbac import BaseRBACAction
from ai.backend.manager.actions.action.rbac_registry import RBAC_ACTION_REGISTRY


def _import_all_rbac_modules() -> None:
"""Import all rbac_* modules to ensure __subclasses__() discovers everything."""
package_path = action_pkg.__path__
for module_info in pkgutil.iter_modules(package_path):
if module_info.name.startswith("rbac_"):
importlib.import_module(f"{action_pkg.__name__}.{module_info.name}")


def _collect_concrete_subclasses(base: type) -> set[type[BaseRBACAction]]:
"""Recursively collect all concrete (non-abstract) subclasses."""
result: set[type[BaseRBACAction]] = set()
for sub in base.__subclasses__():
if not inspect.isabstract(sub):
result.add(sub)
result.update(_collect_concrete_subclasses(sub))
return result


# Ensure all rbac_* modules are imported before any test runs.
_import_all_rbac_modules()


class TestRBACRegistryCompleteness:
def test_all_concrete_subclasses_are_registered(self) -> None:
concrete_subclasses = _collect_concrete_subclasses(BaseRBACAction)
registry_set = set(RBAC_ACTION_REGISTRY)
missing = concrete_subclasses - registry_set
assert not missing, (
f"The following BaseRBACAction subclasses are not in RBAC_ACTION_REGISTRY: "
f"{', '.join(cls.__name__ for cls in sorted(missing, key=lambda c: c.__name__))}"
)

def test_registry_has_no_duplicates(self) -> None:
seen: set[type[BaseRBACAction]] = set()
duplicates: list[str] = []
for cls in RBAC_ACTION_REGISTRY:
if cls in seen:
duplicates.append(cls.__name__)
seen.add(cls)
assert not duplicates, f"Duplicate entries in RBAC_ACTION_REGISTRY: {', '.join(duplicates)}"

def test_registry_contains_only_base_rbac_action_subclasses(self) -> None:
non_subclasses: list[str] = []
for entry in RBAC_ACTION_REGISTRY:
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issubclass(cls, BaseRBACAction) will raise TypeError if an entry in RBAC_ACTION_REGISTRY is not a class/type (e.g., None, a function, etc.), which makes the test error out instead of reporting a clear assertion failure. Consider first checking isinstance(entry, type) (or catching TypeError) and then treating non-types as invalid registry entries so the failure message is actionable.

Suggested change
for entry in RBAC_ACTION_REGISTRY:
for entry in RBAC_ACTION_REGISTRY:
if not isinstance(entry, type):
non_subclasses.append(repr(entry))
continue

Copilot uses AI. Check for mistakes.
cls: type = entry
if not issubclass(cls, BaseRBACAction):
non_subclasses.append(cls.__name__)
assert not non_subclasses, (
f"Non-BaseRBACAction entries in RBAC_ACTION_REGISTRY: {', '.join(non_subclasses)}"
)
Loading