Skip to content
Open
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
1 change: 1 addition & 0 deletions changes/7128.feature.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Migrate VFolder invitations data to RBAC permission groups DB. This will allow VFolder invitees can "list" the scopes of invited VFolder owner.
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
"""migrate invited vfolders to permission groups

Revision ID: e06b026a4578
Revises: b0fb0eb6b6bc
Create Date: 2025-12-04 16:42:17.153498

"""

import enum
import uuid

import sqlalchemy as sa
from alembic import op
from sqlalchemy.engine import Connection
from sqlalchemy.engine.row import Row
from sqlalchemy.orm import registry

from ai.backend.manager.models.base import GUID, EnumValueType, IDColumn, metadata
from ai.backend.manager.models.rbac_models.migration.enums import RoleSource, ScopeType
from ai.backend.manager.models.rbac_models.migration.models import (
get_permission_groups_table,
get_roles_table,
get_user_roles_table,
)
from ai.backend.manager.models.rbac_models.migration.types import (
PermissionGroupCreateInput,
)
from ai.backend.manager.models.rbac_models.migration.utils import (
insert_skip_on_conflict,
)
from ai.backend.manager.models.rbac_models.migration.vfolder import (
VFolderPermission,
)

# revision identifiers, used by Alembic.
revision = "e06b026a4578"
down_revision = "b0fb0eb6b6bc"
branch_labels = None
depends_on = None

mapper_registry = registry(metadata=metadata)


class UserRole(enum.StrEnum):
"""
User's role.
"""

SUPERADMIN = "superadmin"
ADMIN = "admin"
USER = "user"
MONITOR = "monitor"


class Tables:
@staticmethod
def get_vfolder_permissions_table() -> sa.Table:
vfolder_permissions_table = sa.Table(
"vfolder_permissions",
mapper_registry.metadata,
IDColumn("id"),
sa.Column(
"permission",
EnumValueType(VFolderPermission),
default=VFolderPermission.READ_WRITE,
nullable=False,
),
sa.Column(
"vfolder",
GUID,
sa.ForeignKey("vfolders.id", onupdate="CASCADE", ondelete="CASCADE"),
nullable=False,
),
sa.Column("user", GUID, sa.ForeignKey("users.uuid"), nullable=False),
extend_existing=True,
)
return vfolder_permissions_table

@staticmethod
def get_users_table() -> sa.Table:
users_table = sa.Table(
"users",
mapper_registry.metadata,
IDColumn("uuid"),
sa.Column("username", sa.String(length=64), unique=True),
sa.Column("domain_name", sa.String(length=64), index=True),
sa.Column("role", EnumValueType(UserRole), default=UserRole.USER),
extend_existing=True,
)
return users_table


class PermissionCreator:
@classmethod
def add_unique_constraint_to_permission_groups_role_id_scope_id(cls) -> None:
permission_groups_table = get_permission_groups_table()
op.create_unique_constraint(
"uq_permission_groups_role_id_scope_id",
permission_groups_table.name,
["role_id", "scope_id", "scope_type"],
)

@classmethod
def drop_unique_constraint_from_permission_groups_role_id_scope_id(cls) -> None:
permission_groups_table = get_permission_groups_table()
op.drop_constraint(
"uq_permission_groups_role_id_scope_id",
permission_groups_table.name,
type_="unique",
)
Comment on lines +94 to +110
Copy link
Member

Choose a reason for hiding this comment

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

In my personal opinion, having these as separate methods actually hurts readability. The methods are very simple, but their names are too long.


@classmethod
def _query_vfolder_permissions(
cls,
db_conn: Connection,
offset: int,
page_size: int,
) -> list[Row]:
vfolder_permissions_table = Tables.get_vfolder_permissions_table()

users_table = Tables.get_users_table()
user_roles_table = get_user_roles_table()
roles_table = get_roles_table()
stmt = (
sa.select(
vfolder_permissions_table.c.user.label("user_id"),
roles_table.c.id.label("role_id"),
)
.select_from(
sa.join(
vfolder_permissions_table,
users_table,
vfolder_permissions_table.c.user == users_table.c.uuid,
)
.join(
user_roles_table,
user_roles_table.c.user_id == users_table.c.uuid,
)
.join(
roles_table,
roles_table.c.id == user_roles_table.c.role_id,
)
)
.where(roles_table.c.source == RoleSource.SYSTEM)
.offset(offset)
.limit(page_size)
)
return db_conn.execute(stmt).all()
Comment on lines +124 to +148
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The vfolder_permissions table includes a permission column that stores different permission levels (READ_ONLY, READ_WRITE, RW_DELETE), but this migration ignores these permission levels when creating permission groups. All users with vfolder permissions will receive the same permission group regardless of their original permission level. This loses important access control information. The migration should either preserve the permission levels or document why they are being discarded.

Copilot uses AI. Check for mistakes.
Comment on lines +124 to +148
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The query selects users from vfolder_permissions and joins with their system roles, but it doesn't include the vfolder column from vfolder_permissions. This means the migration creates permission groups at the USER scope level regardless of which vfolder the permission was for. This appears to be granting users a role-based permission to all vfolders, rather than preserving the specific vfolder invitations. If the intent is to migrate specific vfolder invitations to permission groups, the query should include the vfolder ID and create permission groups or object permissions that are scoped to specific vfolders.

Copilot uses AI. Check for mistakes.
Comment on lines +124 to +148
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The query doesn't filter or exclude any rows, which means it will attempt to create permission groups for all users who have vfolder permissions. This includes potentially creating duplicate permission groups for the same user if they have permissions on multiple vfolders. While insert_skip_on_conflict handles conflicts, the migration may still create multiple permission groups with identical role_id, scope_type, and scope_id if a user has permissions on multiple vfolders. Consider adding a DISTINCT clause or grouping by (user_id, role_id) to prevent processing the same user-role combination multiple times.

Copilot uses AI. Check for mistakes.

@classmethod
def _invitiee_user_to_permission_group_input(
cls,
role_id: uuid.UUID,
user_id: uuid.UUID,
) -> PermissionGroupCreateInput:
permission_group_input = PermissionGroupCreateInput(
role_id=role_id,
scope_type=ScopeType.USER,
scope_id=str(user_id),
)
return permission_group_input
Comment on lines +150 to +161
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The _invitiee_user_to_permission_group_input method name suggests it's converting invitee user data to permission group input, but there's no documentation explaining what this conversion does or why it's creating a USER-scoped permission group. Adding a docstring would help clarify the intended behavior and migration strategy.

Copilot uses AI. Check for mistakes.

@classmethod
def add_vfolder_permissions_as_permission_groups(cls, db_conn: Connection) -> None:
permission_groups_table = get_permission_groups_table()

offset = 0
page_size = 100
while True:
vp_rows = cls._query_vfolder_permissions(db_conn, offset, page_size)
offset += page_size
if not vp_rows:
break
permission_group_inputs: list[PermissionGroupCreateInput] = []
for row in vp_rows:
input = cls._invitiee_user_to_permission_group_input(
role_id=row.role_id,
user_id=row.user_id,
)
permission_group_inputs.append(input)
Comment on lines +176 to +180
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The variable name input shadows the built-in Python function. Consider using a more descriptive name like permission_input or pg_input to avoid shadowing the built-in.

Suggested change
input = cls._invitiee_user_to_permission_group_input(
role_id=row.role_id,
user_id=row.user_id,
)
permission_group_inputs.append(input)
permission_group_input = cls._invitiee_user_to_permission_group_input(
role_id=row.role_id,
user_id=row.user_id,
)
permission_group_inputs.append(permission_group_input)

Copilot uses AI. Check for mistakes.

insert_skip_on_conflict(db_conn, permission_groups_table, permission_group_inputs)


def upgrade() -> None:
PermissionCreator.add_unique_constraint_to_permission_groups_role_id_scope_id()
conn = op.get_bind()
PermissionCreator.add_vfolder_permissions_as_permission_groups(conn)
PermissionCreator.drop_unique_constraint_from_permission_groups_role_id_scope_id()
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The unique constraint on permission_groups is added before inserting data and then dropped afterward. This pattern is unusual - typically, a unique constraint is either permanently present or not. If the constraint is needed to prevent duplicate insertions during migration, the insert_skip_on_conflict function already handles conflicts. If duplicate permission groups should be prevented permanently, the constraint should remain. The current approach of adding and then removing the constraint suggests unclear intent and could lead to duplicate permission groups being created after the migration completes.

Suggested change
PermissionCreator.drop_unique_constraint_from_permission_groups_role_id_scope_id()

Copilot uses AI. Check for mistakes.


def downgrade() -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Is this change an irreversible migration?

Copy link
Member

Choose a reason for hiding this comment

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

If that’s the case, the PR prefix should be marked as ‘breaking’, or the PR should more explicitly indicate that it is non-reversible.

Copy link
Member Author

Choose a reason for hiding this comment

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

it is not breaking because the RBAC DB is not used in any API yet. This is a preparation step for RBAC implementation

pass
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

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

The downgrade() function is empty, which means this migration cannot be rolled back. If the migration needs to be reversed, all permission groups created by this migration would remain in the database. Consider implementing a downgrade that either removes the migrated permission groups or documents why rollback is not supported.

Suggested change
pass
"""
Remove permission groups created by this migration.
"""
conn = op.get_bind()
permission_groups_table = get_permission_groups_table()
# Delete permission groups with scope_type=USER that were created for vfolder permissions
delete_stmt = (
permission_groups_table.delete()
.where(permission_groups_table.c.scope_type == ScopeType.USER)
)
conn.execute(delete_stmt)

Copilot uses AI. Check for mistakes.
Loading